This revision now requires changes to proceed.
kevincox added inline comments.
kevincox requested changes to this revision.
INLINE COMMENTS
> Alphare wrote in status.rs:83
> Collecting into a `Result` prevents us from doing a `filter_map` during
> `collect`. Implementing a `ParallelIterator` adapter that would do a
> `filter_map` with `Result` handling is possible, but would require more work.
>
> In this specific case, as I eluded to above, I just removed the sentinel
> value so the question is not relevant anymore.
I don't follow. can't this `.filter_map(...)` be replaced with a
`.filter(...).map(...)`? I think this would make the code much more clear.
> status.rs:23
> +fn walk_explicit(
> + files: Vec<HgPathBuf>,
> + dmap: &mut DirstateMap,
This doesn't appear to be consumed by the function so you should take a
`&[HgPathBuf]`. Or since I don't think you use the path either probably a
`&[HgPath]`.
> status.rs:134
> + results: HashMap<HgPathBuf, Option<HgMetadata>>,
> +) -> ((Vec<HgPathBuf>, StatusResult)) {
> + let mut lookup = vec![];
Why are there two pairs of parens?
> status.rs:168
> + _ => {}
> + },
> + Some(HgMetadata {
This seems like a very complicated way to write this. Why not separate the
`match state` and do it first. Or put it in an `else` block? Furthermore why do
you have two separate `match state`? Why not handle them all in one `match`?
> status.rs:240
> + let mut results =
> + walk_explicit(files, &mut dmap, root_dir.as_ref().to_owned())?;
> +
Do you need to do this to `root_dir`? The `walk_explicit` function seems to
only need `AsRef<Path>` so you should be able to just pass it. You might need
to update the argument to be a reference.
> files.rs:112
> + // TODO support other platforms
> + unimplemented!()
> + }
Isn't it better to avoid implementing this so that we get a compile error
rather than a runtime error?
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7058/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7058
To: Alphare, #hg-reviewers, kevincox
Cc: yuja, martinvonz, durin42, kevincox, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
