Page MenuHomePhabricator

rust-hg-path: add method to get part of a path relative to a prefix
ClosedPublic

Authored by Alphare on Fri, Nov 29, 12:57 PM.

Details

Summary

This will be used in the next patch in this series.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Fri, Nov 29, 12:57 PM
kevincox requested changes to this revision.Mon, Dec 2, 8:17 AM
kevincox added inline comments.
rust/hg-core/src/utils/hg_path.rs
131

base.is_empty()? If we don't have that helper we should probably add it.

131

The comment says that base must end with a / however you appear to handle cases where it doesn't Can you either update the documentation or the code?

134

Instead of using .last() can we add and use base.ends_with(b'/')?

134

base.len() > 0 is redundant both with the early return above and because last() will return None if base is empty.

215

I would do self.as_bytes() for consistency.

Or even better I believe you can #[derive(Eq,PartialEq)].

This revision now requires changes to proceed.Mon, Dec 2, 8:17 AM
Alphare marked 4 inline comments as done.Mon, Dec 2, 9:20 AM
Alphare updated this revision to Diff 18422.
Alphare added inline comments.Mon, Dec 2, 9:20 AM
rust/hg-core/src/utils/hg_path.rs
131

I've clarified the documentation.

kevincox accepted this revision.Mon, Dec 2, 10:15 AM
pulkit accepted this revision.Tue, Dec 10, 10:12 AM
This revision is now accepted and ready to land.Tue, Dec 10, 10:12 AM
martinvonz added inline comments.
rust/hg-core/src/utils/hg_path.rs
438

The argument order here feels backwards. I know some frameworks (like Java's JUnit) put the expected value first, but the definition of assert_eq! doesn't seem to have any opinion and https://doc.rust-lang.org/stable/rust-by-example/testing/unit_testing.html puts the actual value first. What do you think about changing the order? That would make it more readable to me.