Replace panics with results & better option types

aw-client-rust/src/blocking.rs

@@ -10,7 +10,7 @@ use super::AwClient as AsyncAwClient;
 
 pub struct AwClient {
     client: AsyncAwClient,
-    pub baseurl: String,
+    pub baseurl: reqwest::Url,
     pub name: String,
     pub hostname: String,
 }
@@ -38,8 +38,8 @@ macro_rules! proxy_method
 }
 
 impl AwClient {
-    pub fn new(ip: &str, port: &str, name: &str) -> AwClient {
-        let async_client = AsyncAwClient::new(ip, port, name);
+    pub fn new(baseurl: reqwest::Url, name: &str, hostname: String) -> AwClient {
+        let async_client = AsyncAwClient::new(baseurl, name, hostname);
 
         AwClient {
             baseurl: async_client.baseurl.clone(),

aw-client-rust/src/lib.rs

@@ -17,7 +17,7 @@ pub use aw_models::{Bucket, BucketMetadata, Event};
 
 pub struct AwClient {
     client: reqwest::Client,
-    pub baseurl: String,
+    pub baseurl: reqwest::Url,
     pub name: String,
     pub hostname: String,
 }
@@ -29,13 +29,11 @@ impl std::fmt::Debug for AwClient {
 }
 
 impl AwClient {
-    pub fn new(ip: &str, port: &str, name: &str) -> AwClient {
-        let baseurl = format!("http://{ip}:{port}");
+    pub fn new(baseurl: reqwest::Url, name: &str, hostname: String) -> AwClient {
         let client = reqwest::Client::builder()
             .timeout(std::time::Duration::from_secs(120))
             .build()
             .unwrap();
-        let hostname = gethostname::gethostname().into_string().unwrap();
         AwClient {
             client,
             baseurl,
@@ -47,7 +45,7 @@ impl AwClient {
     pub async fn get_bucket(&self, bucketname: &str) -> Result<Bucket, reqwest::Error> {
         let url = format!("{}/api/0/buckets/{}", self.baseurl, bucketname);
         let bucket = self
-            .client
+            .client            
             .get(url)
             .send()
             .await?

aw-client-rust/tests/test.rs

@@ -60,10 +60,9 @@ mod test {
 
     #[test]
     fn test_full() {
-        let ip = "127.0.0.1";
-        let port: String = PORT.to_string();
         let clientname = "aw-client-rust-test";
-        let client: AwClient = AwClient::new(ip, &port, clientname);
+        let url = reqwest::Url::parse(format!("http://127.0.0.1:{}", PORT).as_str()).unwrap();
+        let client: AwClient = AwClient::new(url, clientname, gethostname::gethostname().to_str().unwrap().to_string());
 
         let shutdown_handler = setup_testserver();
 

aw-models/src/bucket.rs

@@ -49,7 +49,7 @@ fn test_bucket() {
         id: "id".to_string(),
         _type: "type".to_string(),
         client: "client".to_string(),
-        hostname: "hostname".to_string(),
+        hostname: "hostname".into(),
         created: None,
         data: json_map! {},
         metadata: BucketMetadata::default(),

aw-sync/src/dirs.rs

@@ -1,14 +1,15 @@
 use dirs::home_dir;
+use std::error::Error;
 use std::fs;
 use std::path::PathBuf;
 
 // TODO: This could be refactored to share logic with aw-server/src/dirs.rs
 // TODO: add proper config support
 #[allow(dead_code)]
-pub fn get_config_dir() -> Result<PathBuf, ()> {
-    let mut dir = appdirs::user_config_dir(Some("activitywatch"), None, false)?;
+pub fn get_config_dir() -> Result<PathBuf, Box<dyn Error>> {
+    let mut dir = appdirs::user_config_dir(Some("activitywatch"), None, false).map_err(|_|"Unable to read user config dir")?;
     dir.push("aw-sync");
-    fs::create_dir_all(dir.clone()).expect("Unable to create config dir");
+    fs::create_dir_all(dir.clone())?;
     Ok(dir)
 }
 
@@ -21,7 +22,8 @@ pub fn get_server_config_path(testing: bool) -> Result<PathBuf, ()> {
     }))
 }
 
-pub fn get_sync_dir() -> Result<PathBuf, ()> {
+pub fn get_sync_dir() -> Result<PathBuf, Box<dyn Error>> {
     // TODO: make this configurable
-    home_dir().map(|p| p.join("ActivityWatchSync")).ok_or(())
+    let home_dir = home_dir().ok_or("Unable to read home_dir")?;
+    Ok(home_dir.join("ActivityWatchSync"))
 }

aw-sync/src/main.rs

@@ -14,10 +14,9 @@ extern crate serde;
 extern crate serde_json;
 
 use std::error::Error;
-use std::path::Path;
 use std::path::PathBuf;
 
-use chrono::{DateTime, Datelike, TimeZone, Utc};
+use chrono::{DateTime, Utc};
 use clap::{Parser, Subcommand};
 
 use aw_client_rust::blocking::AwClient;
@@ -40,7 +39,7 @@ struct Opts {
 
     /// Port of instance to connect to.
     #[clap(long)]
-    port: Option<String>,
+    port: Option<u16>,
 
     /// Convenience option for using the default testing host and port.
     #[clap(long)]
@@ -58,8 +57,8 @@ enum Commands {
     /// Pulls remote buckets then pushes local buckets.
     Sync {
         /// Host(s) to pull from, comma separated. Will pull from all hosts if not specified.
-        #[clap(long)]
-        host: Option<String>,
+        #[clap(long, value_parser=parse_list)]
+        host: Option<Vec<String>>,
     },
 
     /// Sync subcommand (advanced)
@@ -73,57 +72,72 @@ enum Commands {
         /// If not specified, start from beginning.
         /// NOTE: might be unstable, as count cannot be used to verify integrity of sync.
         /// Format: YYYY-MM-DD
-        #[clap(long)]
-        start_date: Option<String>,
+        #[clap(long, value_parser=parse_start_date)]
+        start_date: Option<DateTime<Utc>>,
 
         /// Specify buckets to sync using a comma-separated list.
         /// If not specified, all buckets will be synced.
-        #[clap(long)]
-        buckets: Option<String>,
+        #[clap(long, value_parser=parse_list)]
+        buckets: Option<Vec<String>>,
 
         /// Mode to sync in. Can be "push", "pull", or "both".
         /// Defaults to "both".
         #[clap(long, default_value = "both")]
-        mode: String,
+        mode: sync::SyncMode,
 
         /// Full path to sync directory.
         /// If not specified, exit.
         #[clap(long)]
-        sync_dir: String,
+        sync_dir: PathBuf,
 
         /// Full path to sync db file
         /// Useful for syncing buckets from a specific db file in the sync directory.
         /// Must be a valid absolute path to a file in the sync directory.
         #[clap(long)]
-        sync_db: Option<String>,
+        sync_db: Option<PathBuf>,
     },
     /// List buckets and their sync status.
     List {},
 }
 
+fn parse_start_date(arg: &str) -> Result<DateTime<Utc>, chrono::ParseError>
+{
+    chrono::NaiveDate::parse_from_str(arg, "%Y-%m-%d")
+        .map(|nd| {
+            nd.and_time(chrono::NaiveTime::MIN).and_utc()
+        })
+}
+
+fn parse_list(arg: &str) -> Result<Vec<String>, clap::Error>
+{
+    Ok(arg.split(',').map(|s| s.to_string()).collect())
+}
+
 fn main() -> Result<(), Box<dyn Error>> {
     let opts: Opts = Opts::parse();
     let verbose = opts.verbose;
 
     info!("Started aw-sync...");
 
-    aw_server::logging::setup_logger("aw-sync", opts.testing, verbose)
-        .expect("Failed to setup logging");
+    aw_server::logging::setup_logger("aw-sync", opts.testing, verbose)?;
 
     let port = opts
         .port
-        .or_else(|| Some(crate::util::get_server_port(opts.testing).ok()?.to_string()))
-        .unwrap();
+        .map(|a| Ok(a))
+        .unwrap_or_else(||util::get_server_port(opts.testing))?;
+
+    let baseurl = reqwest::Url::parse(&format!("http://{}:{}", opts.host, port))?;
 
-    let client = AwClient::new(opts.host.as_str(), port.as_str(), "aw-sync");
+    let hostname = util::get_hostname()?;
 
-    match &opts.command {
+    let client = AwClient::new(baseurl, "aw-sync", hostname);
+
+    match opts.command {
         // Perform basic sync
         Commands::Sync { host } => {
             // Pull
             match host {
-                Some(host) => {
-                    let hosts: Vec<&str> = host.split(',').collect();
+                Some(hosts) => {
                     for host in hosts.iter() {
                         info!("Pulling from host: {}", host);
                         sync_wrapper::pull(host, &client)?;
@@ -137,8 +151,7 @@ fn main() -> Result<(), Box<dyn Error>> {
 
             // Push
             info!("Pushing local data");
-            sync_wrapper::push(&client)?;
-            Ok(())
+            sync_wrapper::push(&client)
         }
         // Perform two-way sync
         Commands::SyncAdvanced {
@@ -148,60 +161,32 @@ fn main() -> Result<(), Box<dyn Error>> {
             sync_dir,
             sync_db,
         } => {
-            let sync_directory = if sync_dir.is_empty() {
-                error!("No sync directory specified, exiting...");
-                std::process::exit(1);
-            } else {
-                Path::new(&sync_dir)
-            };
-            info!("Using sync dir: {}", sync_directory.display());
 
-            if let Some(sync_db) = &sync_db {
-                info!("Using sync db: {}", sync_db);
+            if !sync_dir.is_absolute() {
+                Err("Sync dir must be absolute")?
             }
 
-            let start: Option<DateTime<Utc>> = start_date.as_ref().map(|date| {
-                println!("{}", date.clone());
-                chrono::NaiveDate::parse_from_str(&date.clone(), "%Y-%m-%d")
-                    .map(|nd| {
-                        Utc.with_ymd_and_hms(nd.year(), nd.month(), nd.day(), 0, 0, 0)
-                            .single()
-                            .unwrap()
-                    })
-                    .expect("Date was not on the format YYYY-MM-DD")
-            });
-
-            // Parse comma-separated list
-            let buckets_vec: Option<Vec<String>> = buckets
-                .as_ref()
-                .map(|b| b.split(',').map(|s| s.to_string()).collect());
-
-            let sync_db: Option<PathBuf> = sync_db.as_ref().map(|db| {
-                let db_path = Path::new(db);
+            info!("Using sync dir: {}", &sync_dir.display());
+
+            if let Some(db_path) = &sync_db {
+                info!("Using sync db: {}", &db_path.display());
+
                 if !db_path.is_absolute() {
-                    panic!("Sync db path must be absolute");
+                    Err("Sync db path must be absolute")?
                 }
-                if !db_path.starts_with(sync_directory) {
-                    panic!("Sync db path must be in sync directory");
+                if !db_path.starts_with(&sync_dir) {
+                    Err("Sync db path must be in sync directory")?
                 }
-                db_path.to_path_buf()
-            });
+            }
 
             let sync_spec = sync::SyncSpec {
-                path: sync_directory.to_path_buf(),
+                path: sync_dir,
                 path_db: sync_db,
-                buckets: buckets_vec,
-                start,
-            };
-
-            let mode_enum = match mode.as_str() {
-                "push" => sync::SyncMode::Push,
-                "pull" => sync::SyncMode::Pull,
-                "both" => sync::SyncMode::Both,
-                _ => panic!("Invalid mode"),
+                buckets,
+                start: start_date,
             };
 
-            sync::sync_run(&client, &sync_spec, mode_enum)
+            sync::sync_run(&client, &sync_spec, mode)
         }
 
         // List all buckets

aw-sync/src/sync.rs

@@ -9,6 +9,7 @@ extern crate chrono;
 extern crate reqwest;
 extern crate serde_json;
 
+use std::error::Error;
 use std::fs;
 use std::path::{Path, PathBuf};
 
@@ -17,10 +18,12 @@ use chrono::{DateTime, Utc};
 
 use aw_datastore::{Datastore, DatastoreError};
 use aw_models::{Bucket, Event};
+use clap::ValueEnum;
 
 use crate::accessmethod::AccessMethod;
 
-#[derive(PartialEq, Eq)]
+#[derive(PartialEq, Eq, Copy, Clone)]
+#[derive(ValueEnum)]
 pub enum SyncMode {
     Push,
     Pull,
@@ -54,8 +57,8 @@ impl Default for SyncSpec {
 }
 
 /// Performs a single sync pass
-pub fn sync_run(client: &AwClient, sync_spec: &SyncSpec, mode: SyncMode) -> Result<(), String> {
-    let info = client.get_info().map_err(|e| e.to_string())?;
+pub fn sync_run(client: &AwClient, sync_spec: &SyncSpec, mode: SyncMode) -> Result<(), Box<dyn Error>> {
+    let info = client.get_info()?;
 
     // FIXME: Here it is assumed that the device_id for the local server is the one used by
     // aw-server-rust, which is not necessarily true (aw-server-python has seperate device_id).
@@ -128,10 +131,10 @@ pub fn sync_run(client: &AwClient, sync_spec: &SyncSpec, mode: SyncMode) -> Resu
 }
 
 #[allow(dead_code)]
-pub fn list_buckets(client: &AwClient) -> Result<(), String> {
+pub fn list_buckets(client: &AwClient) -> Result<(), Box<dyn Error>> {
     let sync_directory = crate::dirs::get_sync_dir().map_err(|_| "Could not get sync dir")?;
     let sync_directory = sync_directory.as_path();
-    let info = client.get_info().map_err(|e| e.to_string())?;
+    let info = client.get_info()?;
 
     // FIXME: Incorrect device_id assumption?
     let device_id = info.device_id.as_str();
@@ -156,12 +159,12 @@ pub fn list_buckets(client: &AwClient) -> Result<(), String> {
     Ok(())
 }
 
-fn setup_local_remote(path: &Path, device_id: &str) -> Result<Datastore, String> {
+fn setup_local_remote(path: &Path, device_id: &str) -> Result<Datastore, Box<dyn Error>> {
     // FIXME: Don't run twice if already exists
-    fs::create_dir_all(path).unwrap();
+    fs::create_dir_all(path)?;
 
     let remotedir = path.join(device_id);
-    fs::create_dir_all(&remotedir).unwrap();
+    fs::create_dir_all(&remotedir)?;
 
     let dbfile = remotedir.join("test.db");