This is an archive of the discontinued Mercurial Phabricator instance.

rust-hg-path: add useful methods to `HgPath`
ClosedPublic

Authored by Alphare on Jan 14 2020, 12:33 PM.

Details

Summary

This changeset introduces the use of the pretty_assertions crate for easier
to read test output.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

Alphare created this revision.Jan 14 2020, 12:33 PM
kevincox accepted this revision.Jan 15 2020, 9:59 AM
kevincox added inline comments.
rust/hg-core/src/utils/hg_path.rs
196

It would be nice to have a trim_trailing_slash helper.

Alphare marked an inline comment as done.Jan 15 2020, 4:10 PM
Alphare updated this revision to Diff 19318.Jan 15 2020, 4:55 PM
kevincox accepted this revision.Jan 16 2020, 5:00 AM
Alphare updated this revision to Diff 19352.Jan 16 2020, 5:21 AM
kevincox requested changes to this revision.Jan 16 2020, 10:51 AM
kevincox added inline comments.
rust/hg-core/src/utils/hg_path.rs
169

I don't think we should have this method. It provides us no way to ensure that the invariants of this type are upheld. It is slightly more typing but I think we should recommend converting to a slice, doing the mutation, then converting back to HgPath then we can at least to validation (possibly only in debug builds) in the constructor.

This revision now requires changes to proceed.Jan 16 2020, 10:51 AM
Alphare added inline comments.Jan 16 2020, 1:22 PM
rust/hg-core/src/utils/hg_path.rs
169

Fair enough.

Alphare updated this revision to Diff 19382.Jan 16 2020, 4:49 PM
kevincox accepted this revision.Jan 17 2020, 4:44 AM
pulkit accepted this revision.Jan 21 2020, 7:08 AM
This revision is now accepted and ready to land.Jan 21 2020, 7:08 AM
martinvonz added inline comments.
rust/hg-core/src/utils/hg_path.rs
176–183

When do we create such paths? I can imagine using them for referring to directories. Is that it? If it's some other use-case, I wonder if we can instead prevent the trailing slash from entering the HgPath.

Alphare added inline comments.Jan 24 2020, 4:49 AM
rust/hg-core/src/utils/hg_path.rs
176–183

Maybe, but I'm not sure it's worth risking a weird edge-case bug or losing performance over (possibly negligible). What do you think?

martinvonz added inline comments.Jan 24 2020, 9:12 AM
rust/hg-core/src/utils/hg_path.rs
176–183

Another way of thinking about my question: what happens if we never call this function? Which tests fail?

Alphare added inline comments.Jan 24 2020, 9:18 AM
rust/hg-core/src/utils/hg_path.rs
176–183

This is currently this only place it's called (line 189), it was refactored for readability first I suppose.

martinvonz added inline comments.Jan 24 2020, 9:50 AM
rust/hg-core/src/utils/hg_path.rs
176–183

And what happens if you remove it?

martinvonz requested changes to this revision.Jan 28 2020, 12:20 PM
This revision now requires changes to proceed.Jan 28 2020, 12:20 PM
Alphare added inline comments.Feb 6 2020, 10:03 AM
rust/hg-core/src/utils/hg_path.rs
176–183

It only breaks the single rust test I had to check for that particular case. I'll remove it.

martinvonz added inline comments.Feb 6 2020, 11:12 AM
rust/hg-core/src/utils/hg_path.rs
192

Might be good to add an assert! here in case my assertion (that HgPath should never have a trailing /) is not correct.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.