[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