Less reask for file changes #722
Conversation
src/lib.rs
Outdated
| debug!("Saving metadata"); | ||
| timeline::save_options(&used_variables, &ctx.cmd_opt.src, &ctx.cmd_opt.dst_folder)?; | ||
|
|
||
| timeline::save_metas_for_source(new_metas, &ctx.cmd_opt.dst_folder, "global".into())?; |
There was a problem hiding this comment.
🤌 nitpick: If you transform a &str to a String, You can use String::from("global") or "global".to_string() to be more explicit for the reader (and the compiler)
src/lib.rs
Outdated
| @@ -186,14 +190,40 @@ fn plan(ctx: &Ctx, source_files: Vec<SourceFile>, variables: &Variables) -> Resu | |||
| Ok(actions) | |||
| } | |||
|
|
|||
| fn decide_update_mode( | |||
There was a problem hiding this comment.
💭 thought: I do not like this method, every argument has the same type, so it's prone to error (caller can easily swap 2 args.)
Because this function is private, that's not a big deal.
src/lib.rs
Outdated
| ctx: &Ctx, | ||
| actions: &[Action], | ||
| variables: &Variables, | ||
| ) -> Result<Vec<(FileMeta, FileMeta)>> { |
There was a problem hiding this comment.
🤌 nitpick: maybe return a struct instead of the tuple to add meaningful name (see previous comment)
| //TODO accumulate Result (and error) | ||
| fn execute(ctx: &Ctx, actions: &[Action], variables: &Variables) -> Result<()> { | ||
| fn execute( |
There was a problem hiding this comment.
🤌 nitpick: my damaged-by-Java mind prefers to see a method on Ctx instead of that function.
Just for your information, you can have multiple impl blocs in Rust, even in different modules. (but the type should be define in the crate)
src/lib.rs
Outdated
| // let source = &ctx.cmd_opt.src; | ||
| let target_folder = &ctx.cmd_opt.dst_folder; | ||
| let past_metas = timeline::get_stored_metas_for_source(target_folder, "global".into())?; | ||
| let mut new_metas = Vec::new(); |
There was a problem hiding this comment.
🤌 nitpick: here I prefer vec![] or a Vec::with_capacity(actions.len())
src/lib.rs
Outdated
| @@ -365,6 +415,10 @@ where | |||
| let src = src.as_ref(); | |||
| loop { | |||
| match mode { | |||
| UpdateMode::Auto => { | |||
| // should not enter here | |||
There was a problem hiding this comment.
🤌 nitpick: if you are sure of that, use the unreachable!() macro (it's panicking)
src/timeline/files.rs
Outdated
| let tracked = load_tracked(target_folder)?; | ||
| let infos = tracked | ||
| .files | ||
| .get(&source) |
There was a problem hiding this comment.
🎯 suggestion: you can avoid the cloned if you remove instead of the get
src/timeline/files.rs
Outdated
| } | ||
|
|
||
| fn save_tracked(tracked: &TrackedFiles, target_folder: &Path) -> Result<()> { | ||
| serde_json::to_writer( |
There was a problem hiding this comment.
🎯 suggestion: introduce local variables to avoid this nesting
src/timeline/files.rs
Outdated
| } | ||
|
|
||
| pub(crate) fn get_meta(base_folder: &Path, relative_path: &Path) -> Result<FileMeta> { | ||
| let h = Code::Sha2_256.digest(&fs::read(base_folder.join(relative_path))?); |
There was a problem hiding this comment.
💭 thought: need to fully read the file to compute the hash.
Maybe there is a way to use a buffered reader?
src/timeline/files.rs
Outdated
| pub(crate) fn get_meta(base_folder: &Path, relative_path: &Path) -> Result<FileMeta> { | ||
| let h = Code::Sha2_256.digest(&fs::read(base_folder.join(relative_path))?); | ||
| let info = FileMeta { | ||
| key: relative_path.to_string_lossy().to_string(), |
There was a problem hiding this comment.
💭 thought: Why not use a PathBuf in the `FileMeta?
| fn from(variables: Variables) -> Self { | ||
| variables | ||
| .tree() | ||
| .iter() |
There was a problem hiding this comment.
💭 thought: maybe there is an into_iter() to avoid clone ?
No description provided.