Skip to content

Commit dc70318

Browse files
nathanmerrillErikBjare
nathanmerrill
and
ErikBjare
authored
fix: replace panics with results & better option types (#437)
* Replace panics with results & better option types * Apply suggestions from code review * Remove Reqwest from AWClient::new and return a Result instead * Run cargo fmt --------- Co-authored-by: NathanM <[email protected]> Co-authored-by: Erik Bjäreholt <[email protected]>
1 parent a144746 commit dc70318

File tree

9 files changed

+114
-137
lines changed

9 files changed

+114
-137
lines changed

‎aw-client-rust/src/blocking.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
use std::collections::HashMap;
21
use std::future::Future;
32
use std::vec::Vec;
3+
use std::{collections::HashMap, error::Error};
44

55
use chrono::{DateTime, Utc};
66

@@ -10,7 +10,7 @@ use super::AwClient as AsyncAwClient;
1010

1111
pub struct AwClient {
1212
client: AsyncAwClient,
13-
pub baseurl: String,
13+
pub baseurl: reqwest::Url,
1414
pub name: String,
1515
pub hostname: String,
1616
}
@@ -38,15 +38,15 @@ macro_rules! proxy_method
3838
}
3939

4040
impl AwClient {
41-
pub fn new(ip: &str, port: &str, name: &str) -> AwClient {
42-
let async_client = AsyncAwClient::new(ip, port, name);
41+
pub fn new(host: &str, port: u16, name: &str) -> Result<AwClient, Box<dyn Error>> {
42+
let async_client = AsyncAwClient::new(host, port, name)?;
4343

44-
AwClient {
44+
Ok(AwClient {
4545
baseurl: async_client.baseurl.clone(),
4646
name: async_client.name.clone(),
4747
hostname: async_client.hostname.clone(),
4848
client: async_client,
49-
}
49+
})
5050
}
5151

5252
proxy_method!(get_bucket, Bucket, bucketname: &str);

‎aw-client-rust/src/lib.rs‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ extern crate tokio;
77

88
pub mod blocking;
99

10-
use std::collections::HashMap;
1110
use std::vec::Vec;
11+
use std::{collections::HashMap, error::Error};
1212

1313
use chrono::{DateTime, Utc};
1414
use serde_json::Map;
@@ -17,7 +17,7 @@ pub use aw_models::{Bucket, BucketMetadata, Event};
1717

1818
pub struct AwClient {
1919
client: reqwest::Client,
20-
pub baseurl: String,
20+
pub baseurl: reqwest::Url,
2121
pub name: String,
2222
pub hostname: String,
2323
}
@@ -28,20 +28,24 @@ impl std::fmt::Debug for AwClient {
2828
}
2929
}
3030

31+
fn get_hostname() -> String {
32+
return gethostname::gethostname().to_string_lossy().to_string();
33+
}
34+
3135
impl AwClient {
32-
pub fn new(ip: &str, port: &str, name: &str) -> AwClient {
33-
let baseurl = format!("http://{ip}:{port}");
36+
pub fn new(host: &str, port: u16, name: &str) -> Result<AwClient, Box<dyn Error>> {
37+
let baseurl = reqwest::Url::parse(&format!("http://{}:{}", host, port))?;
38+
let hostname = get_hostname();
3439
let client = reqwest::Client::builder()
3540
.timeout(std::time::Duration::from_secs(120))
36-
.build()
37-
.unwrap();
38-
let hostname = gethostname::gethostname().into_string().unwrap();
39-
AwClient {
41+
.build()?;
42+
43+
Ok(AwClient {
4044
client,
4145
baseurl,
4246
name: name.to_string(),
4347
hostname,
44-
}
48+
})
4549
}
4650

4751
pub async fn get_bucket(&self, bucketname: &str) -> Result<Bucket, reqwest::Error> {

‎aw-client-rust/tests/test.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,10 @@ mod test {
6060

6161
#[test]
6262
fn test_full() {
63-
let ip = "127.0.0.1";
64-
let port: String = PORT.to_string();
6563
let clientname = "aw-client-rust-test";
66-
let client: AwClient = AwClient::new(ip, &port, clientname);
64+
65+
let client: AwClient =
66+
AwClient::new("127.0.0.1", PORT, clientname).expect("Client creation failed");
6767

6868
let shutdown_handler = setup_testserver();
6969

‎aw-models/src/bucket.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ fn test_bucket() {
4949
id: "id".to_string(),
5050
_type: "type".to_string(),
5151
client: "client".to_string(),
52-
hostname: "hostname".to_string(),
52+
hostname: "hostname".into(),
5353
created: None,
5454
data: json_map! {},
5555
metadata: BucketMetadata::default(),

‎aw-sync/src/dirs.rs‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,16 @@
11
use dirs::home_dir;
2+
use std::error::Error;
23
use std::fs;
34
use std::path::PathBuf;
45

56
// TODO: This could be refactored to share logic with aw-server/src/dirs.rs
67
// TODO: add proper config support
78
#[allow(dead_code)]
8-
pub fn get_config_dir() -> Result<PathBuf, ()> {
9-
let mut dir = appdirs::user_config_dir(Some("activitywatch"), None, false)?;
9+
pub fn get_config_dir() -> Result<PathBuf, Box<dyn Error>> {
10+
let mut dir = appdirs::user_config_dir(Some("activitywatch"), None, false)
11+
.map_err(|_| "Unable to read user config dir")?;
1012
dir.push("aw-sync");
11-
fs::create_dir_all(dir.clone()).expect("Unable to create config dir");
13+
fs::create_dir_all(dir.clone())?;
1214
Ok(dir)
1315
}
1416

@@ -21,7 +23,8 @@ pub fn get_server_config_path(testing: bool) -> Result<PathBuf, ()> {
2123
}))
2224
}
2325

24-
pub fn get_sync_dir() -> Result<PathBuf, ()> {
26+
pub fn get_sync_dir() -> Result<PathBuf, Box<dyn Error>> {
2527
// TODO: make this configurable
26-
home_dir().map(|p| p.join("ActivityWatchSync")).ok_or(())
28+
let home_dir = home_dir().ok_or("Unable to read home_dir")?;
29+
Ok(home_dir.join("ActivityWatchSync"))
2730
}

‎aw-sync/src/main.rs‎

Lines changed: 42 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,9 @@ extern crate serde;
1414
extern crate serde_json;
1515

1616
use std::error::Error;
17-
use std::path::Path;
1817
use std::path::PathBuf;
1918

20-
use chrono::{DateTime, Datelike, TimeZone, Utc};
19+
use chrono::{DateTime, Utc};
2120
use clap::{Parser, Subcommand};
2221

2322
use aw_client_rust::blocking::AwClient;
@@ -40,7 +39,7 @@ struct Opts {
4039

4140
/// Port of instance to connect to.
4241
#[clap(long)]
43-
port: Option<String>,
42+
port: Option<u16>,
4443

4544
/// Convenience option for using the default testing host and port.
4645
#[clap(long)]
@@ -58,8 +57,8 @@ enum Commands {
5857
/// Pulls remote buckets then pushes local buckets.
5958
Sync {
6059
/// Host(s) to pull from, comma separated. Will pull from all hosts if not specified.
61-
#[clap(long)]
62-
host: Option<String>,
60+
#[clap(long, value_parser=parse_list)]
61+
host: Option<Vec<String>>,
6362
},
6463

6564
/// Sync subcommand (advanced)
@@ -73,57 +72,64 @@ enum Commands {
7372
/// If not specified, start from beginning.
7473
/// NOTE: might be unstable, as count cannot be used to verify integrity of sync.
7574
/// Format: YYYY-MM-DD
76-
#[clap(long)]
77-
start_date: Option<String>,
75+
#[clap(long, value_parser=parse_start_date)]
76+
start_date: Option<DateTime<Utc>>,
7877

7978
/// Specify buckets to sync using a comma-separated list.
8079
/// If not specified, all buckets will be synced.
81-
#[clap(long)]
82-
buckets: Option<String>,
80+
#[clap(long, value_parser=parse_list)]
81+
buckets: Option<Vec<String>>,
8382

8483
/// Mode to sync in. Can be "push", "pull", or "both".
8584
/// Defaults to "both".
8685
#[clap(long, default_value = "both")]
87-
mode: String,
86+
mode: sync::SyncMode,
8887

8988
/// Full path to sync directory.
9089
/// If not specified, exit.
9190
#[clap(long)]
92-
sync_dir: String,
91+
sync_dir: PathBuf,
9392

9493
/// Full path to sync db file
9594
/// Useful for syncing buckets from a specific db file in the sync directory.
9695
/// Must be a valid absolute path to a file in the sync directory.
9796
#[clap(long)]
98-
sync_db: Option<String>,
97+
sync_db: Option<PathBuf>,
9998
},
10099
/// List buckets and their sync status.
101100
List {},
102101
}
103102

103+
fn parse_start_date(arg: &str) -> Result<DateTime<Utc>, chrono::ParseError> {
104+
chrono::NaiveDate::parse_from_str(arg, "%Y-%m-%d")
105+
.map(|nd| nd.and_time(chrono::NaiveTime::MIN).and_utc())
106+
}
107+
108+
fn parse_list(arg: &str) -> Result<Vec<String>, clap::Error> {
109+
Ok(arg.split(',').map(|s| s.to_string()).collect())
110+
}
111+
104112
fn main() -> Result<(), Box<dyn Error>> {
105113
let opts: Opts = Opts::parse();
106114
let verbose = opts.verbose;
107115

108116
info!("Started aw-sync...");
109117

110-
aw_server::logging::setup_logger("aw-sync", opts.testing, verbose)
111-
.expect("Failed to setup logging");
118+
aw_server::logging::setup_logger("aw-sync", opts.testing, verbose)?;
112119

113120
let port = opts
114121
.port
115-
.or_else(|| Some(crate::util::get_server_port(opts.testing).ok()?.to_string()))
116-
.unwrap();
122+
.map(|a| Ok(a))
123+
.unwrap_or_else(|| util::get_server_port(opts.testing))?;
117124

118-
let client = AwClient::new(opts.host.as_str(), port.as_str(), "aw-sync");
125+
let client = AwClient::new(&opts.host, port, "aw-sync")?;
119126

120-
match &opts.command {
127+
match opts.command {
121128
// Perform basic sync
122129
Commands::Sync { host } => {
123130
// Pull
124131
match host {
125-
Some(host) => {
126-
let hosts: Vec<&str> = host.split(',').collect();
132+
Some(hosts) => {
127133
for host in hosts.iter() {
128134
info!("Pulling from host: {}", host);
129135
sync_wrapper::pull(host, &client)?;
@@ -137,8 +143,7 @@ fn main() -> Result<(), Box<dyn Error>> {
137143

138144
// Push
139145
info!("Pushing local data");
140-
sync_wrapper::push(&client)?;
141-
Ok(())
146+
sync_wrapper::push(&client)
142147
}
143148
// Perform two-way sync
144149
Commands::SyncAdvanced {
@@ -148,60 +153,31 @@ fn main() -> Result<(), Box<dyn Error>> {
148153
sync_dir,
149154
sync_db,
150155
} => {
151-
let sync_directory = if sync_dir.is_empty() {
152-
error!("No sync directory specified, exiting...");
153-
std::process::exit(1);
154-
} else {
155-
Path::new(&sync_dir)
156-
};
157-
info!("Using sync dir: {}", sync_directory.display());
158-
159-
if let Some(sync_db) = &sync_db {
160-
info!("Using sync db: {}", sync_db);
156+
if !sync_dir.is_absolute() {
157+
Err("Sync dir must be absolute")?
161158
}
162159

163-
let start: Option<DateTime<Utc>> = start_date.as_ref().map(|date| {
164-
println!("{}", date.clone());
165-
chrono::NaiveDate::parse_from_str(&date.clone(), "%Y-%m-%d")
166-
.map(|nd| {
167-
Utc.with_ymd_and_hms(nd.year(), nd.month(), nd.day(), 0, 0, 0)
168-
.single()
169-
.unwrap()
170-
})
171-
.expect("Date was not on the format YYYY-MM-DD")
172-
});
173-
174-
// Parse comma-separated list
175-
let buckets_vec: Option<Vec<String>> = buckets
176-
.as_ref()
177-
.map(|b| b.split(',').map(|s| s.to_string()).collect());
178-
179-
let sync_db: Option<PathBuf> = sync_db.as_ref().map(|db| {
180-
let db_path = Path::new(db);
160+
info!("Using sync dir: {}", &sync_dir.display());
161+
162+
if let Some(db_path) = &sync_db {
163+
info!("Using sync db: {}", &db_path.display());
164+
181165
if !db_path.is_absolute() {
182-
panic!("Sync db path must be absolute");
166+
Err("Sync db path must be absolute")?
183167
}
184-
if !db_path.starts_with(sync_directory) {
185-
panic!("Sync db path must be in sync directory");
168+
if !db_path.starts_with(&sync_dir) {
169+
Err("Sync db path must be in sync directory")?
186170
}
187-
db_path.to_path_buf()
188-
});
171+
}
189172

190173
let sync_spec = sync::SyncSpec {
191-
path: sync_directory.to_path_buf(),
174+
path: sync_dir,
192175
path_db: sync_db,
193-
buckets: buckets_vec,
194-
start,
195-
};
196-
197-
let mode_enum = match mode.as_str() {
198-
"push" => sync::SyncMode::Push,
199-
"pull" => sync::SyncMode::Pull,
200-
"both" => sync::SyncMode::Both,
201-
_ => panic!("Invalid mode"),
176+
buckets,
177+
start: start_date,
202178
};
203179

204-
sync::sync_run(&client, &sync_spec, mode_enum)
180+
sync::sync_run(&client, &sync_spec, mode)
205181
}
206182

207183
// List all buckets

0 commit comments

Comments
 (0)