[Commented On] D12380: contrib: add a partial-merge tool for sorted lists (such as Python imports)
Alphare (Raphaël Gomès)
phabricator at mercurial-scm.org
Tue Mar 22 09:42:10 UTC 2022
Alphare added a comment.
Alphare accepted this revision as: Alphare.
Accepting this as myself to give you and opportunity to answer my suggestions before taking this. This is a `contrib` patch, so there is no reason that any of these should be blockers.
INLINE COMMENTS
> Cargo.toml:9
> +edition = "2021"
> +# We need https://github.com/rust-lang/rust/pull/89825
> +rust-version = "1.59"
It would be pretty low effort to still support lower versions of Rust, but going to 1.59 allows the use of edition 2021 and makes sense for a `contrib` file anyway, so I understand the appeal of a newer version.
> main.rs:147
> +fn main() {
> + let app = clap::Command::new("merge-lists")
> + .arg(clap::Arg::new("local").index(1).required(true))
Suggestion: with `clap` 3 you can use struct-based arguments. I find the declarative approach clearer and less prone to bugs.
> main.rs:153
> +
> + let base_path = PathBuf::from(matches.value_of("base").unwrap());
> + let local_path = PathBuf::from(matches.value_of("local").unwrap());
Should probably use https://docs.rs/clap/latest/clap/struct.ArgMatches.html#method.value_of_os instead of `value_of`
> main.rs:166
> + // Write out the result if anything changed
> + if new_base_bytes != base_bytes
> + || new_local_bytes != local_bytes
Maybe only write the files that have indeed changed?
> main.rs:189
> + .start
> + .wrapping_add(self.offsets[side] as usize)
> + }
Why do we want the wrapping behavior here?
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D12380/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D12380
To: martinvonz, #hg-reviewers, Alphare
Cc: Alphare, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20220322/75e83e08/attachment-0002.html>
More information about the Mercurial-patches
mailing list