This is an archive of the discontinued Mercurial Phabricator instance.

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.Dec 2 2019, 8:17 AM

Alphare marked 4 inline comments as done.Dec 2 2019, 9:20 AM

Alphare updated this revision to Diff 18422.

Alphare added inline comments.Dec 2 2019, 9:20 AM

rust/hg-core/src/utils/hg_path.rs
131	I've clarified the documentation.

kevincox accepted this revision.Dec 2 2019, 10:15 AM

pulkit accepted this revision.Dec 10 2019, 10:12 AM

This revision is now accepted and ready to land.Dec 10 2019, 10:12 AM

Alphare added a commit: rHG4f1543a2f5c3: rust-hg-path: add method to get part of a path relative to a prefix.Dec 10 2019, 11:17 AM

Closed by commit rHG4f1543a2f5c3: rust-hg-path: add method to get part of a path relative to a prefix (authored by Alphare). · Explain Why

This revision was automatically updated to reflect the committed changes.

martinvonz added a subscriber: martinvonz.Dec 10 2019, 1:04 PM

martinvonz added inline comments.

rust/hg-core/src/utils/hg_path.rs
448	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.

Alphare added inline comments.Dec 11 2019, 3:42 AM

rust/hg-core/src/utils/hg_path.rs
448	It looks that I took the habit because the test runner in PyCharm that I used when starting with Rust prints `expected` and `actual` in that order. I can reverse it if you really think it's more readable this way.

Revision Contents
Changeset List

			Path	Packages
M			rust/hg-core/src/utils/hg_path.rs (55 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	18402		Nov 29 2019, 12:56 PM	★	★
Diff 2	18422		Dec 2 2019, 9:20 AM	★	★
Diff 3	18584	rHG4f1543a2f5c3faea88b7ccad268682afb45f9219	Nov 29 2019, 11:24 AM	★	★

Commit	Parents	Author	Summary	Date
d922ce980ae2	8a8efe0bdb6e	Raphaël Gomès		Nov 29 2019, 11:24 AM

Status	Author	Revision
Closed	Alphare	D7531 rust-matchers: add support for `exactmatcher` in `dirstate.status`
Closed	Alphare	D7530 rust-dirstate-status: update bridge for new rust version of `dirstate.status`
Closed	Alphare	D7529 rust-dirstate-status: add `walk_explicit` implementation, use `Matcher` trait
Closed	Alphare	D7528 rust-matchers: add `FileMatcher` implementation
Closed	Alphare	D7527 rust-matchers: add doctests for `AlwaysMatcher`
Closed	Alphare	D7526 rust-hg-path: add method to get part of a path relative to a prefix
Closed	Alphare	D7525 rust-matchers: improve `Matcher` trait ergonomics
Closed	Alphare	D7524 rust-dirs-multiset: use `AsRef` instead of concrete types when possible
Closed	Alphare	D7523 rust-hg-path: implement `Display` for `HgPath` and `HgPathBuf`
Closed	Alphare	D7119 rust-dirstatemap: remove additional lookup in dirstate.matches
Abandoned	Alphare	D7118 rust-dirstatemap: remove additional lookups in traverse
Closed	Alphare	D7116 rust-performance: introduce FastHashMap type alias for HashMap

Diff 18402

rust/hg-core/src/utils/hg_path.rs

	HgPathBuf::from(self.inner.to_ascii_lowercase())			HgPathBuf::from(self.inner.to_ascii_lowercase())
	}			}
	pub fn as_bytes(&self) -> &[u8] {			pub fn as_bytes(&self) -> &[u8] {
	&self.inner			&self.inner
	}			}
	pub fn contains(&self, other: u8) -> bool {			pub fn contains(&self, other: u8) -> bool {
	self.inner.contains(&other)			self.inner.contains(&other)
	}			}
				pub fn starts_with(&self, needle: impl AsRef<HgPath>) -> bool {
				self.inner.starts_with(needle.as_ref().as_bytes())
				}
	pub fn join<T: ?Sized + AsRef<HgPath>>(&self, other: &T) -> HgPathBuf {			pub fn join<T: ?Sized + AsRef<HgPath>>(&self, other: &T) -> HgPathBuf {
	let mut inner = self.inner.to_owned();			let mut inner = self.inner.to_owned();
	if inner.len() != 0 && inner.last() != Some(&b'/') {			if inner.len() != 0 && inner.last() != Some(&b'/') {
	inner.push(b'/');			inner.push(b'/');
	}			}
	inner.extend(other.as_ref().bytes());			inner.extend(other.as_ref().bytes());
	HgPathBuf::from_bytes(&inner)			HgPathBuf::from_bytes(&inner)
	}			}
				/// Given a base directory (has to end with a `b'/'`), returns the slice
				/// of `self` relative to the base directory. If `base` is not a directory,
				/// returns `None`.
				pub fn relative_to(&self, base: impl AsRef<HgPath>) -> Option<&HgPath> {
				let base = base.as_ref();
				if base.len() == 0 {
				kevincoxUnsubmitted Done `base.is_empty()`? If we don't have that helper we should probably add it. kevincox: `base.is_empty()`? If we don't have that helper we should probably add it.
				kevincoxUnsubmitted Not Done 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? kevincox: The comment says that `base` must end with a `/` however you appear to handle cases where it…
				AlphareAuthorUnsubmitted Done I've clarified the documentation. Alphare: I've clarified the documentation.
				return Some(self);
				}
				let is_dir = base.len() > 0 && base.as_bytes().last() == Some(&b'/');
				kevincoxUnsubmitted Done Instead of using `.last()` can we add and use `base.ends_with(b'/')`? kevincox: Instead of using `.last()` can we add and use `base.ends_with(b'/')`?
				kevincoxUnsubmitted Done `base.len() > 0` is redundant both with the early return above and because `last()` will return `None` if `base` is empty. kevincox: `base.len() > 0` is redundant both with the early return above and because `last()` will return…
				if is_dir && self.starts_with(base) {
				Some(HgPath::new(&self.inner[base.len()..]))
				} else {
				None
				}
				}
	/// Checks for errors in the path, short-circuiting at the first one.			/// Checks for errors in the path, short-circuiting at the first one.
	/// This generates fine-grained errors useful for debugging.			/// This generates fine-grained errors useful for debugging.
	/// To simply check if the path is valid during tests, use `is_valid`.			/// To simply check if the path is valid during tests, use `is_valid`.
	pub fn check_state(&self) -> Result<(), HgPathError> {			pub fn check_state(&self) -> Result<(), HgPathError> {
	if self.len() == 0 {			if self.len() == 0 {
	return Ok(());			return Ok(());
	}			}
	let bytes = self.as_bytes();			let bytes = self.as_bytes();
	pub fn into_vec(self) -> Vec<u8> {			pub fn into_vec(self) -> Vec<u8> {
	self.inner			self.inner
	}			}
	pub fn as_ref(&self) -> &[u8] {			pub fn as_ref(&self) -> &[u8] {
	self.inner.as_ref()			self.inner.as_ref()
	}			}
	}			}

				impl PartialEq<HgPath> for HgPathBuf {
				fn eq(&self, other: &HgPath) -> bool {
				self.inner == other.as_bytes()
				kevincoxUnsubmitted Done I would do `self.as_bytes()` for consistency. Or even better I believe you can `#[derive(Eq,PartialEq)]`. kevincox: I would do `self.as_bytes()` for consistency. Or even better I believe you can `#[derive(Eq…
				}
				}

	impl fmt::Display for HgPathBuf {			impl fmt::Display for HgPathBuf {
	fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {			fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
	write!(			write!(
	f,			f,
	"HgPathBuf {{ {} }}",			"HgPathBuf {{ {} }}",
	String::from_utf8_lossy(&self.inner)			String::from_utf8_lossy(&self.inner)
	)			)
	}			}
	// the extra slash? Should we change the signature to a problematic			// the extra slash? Should we change the signature to a problematic
	// `Result<HgPathBuf, HgPathError>`, or should we just keep it so and			// `Result<HgPathBuf, HgPathError>`, or should we just keep it so and
	// let the error happen upon filesystem interaction?			// let the error happen upon filesystem interaction?
	let path = HgPathBuf::from_bytes(b"a/").join(HgPath::new(b"/b"));			let path = HgPathBuf::from_bytes(b"a/").join(HgPath::new(b"/b"));
	assert_eq!(b"a//b", path.as_bytes());			assert_eq!(b"a//b", path.as_bytes());
	let path = HgPathBuf::from_bytes(b"a").join(HgPath::new(b"/b"));			let path = HgPathBuf::from_bytes(b"a").join(HgPath::new(b"/b"));
	assert_eq!(b"a//b", path.as_bytes());			assert_eq!(b"a//b", path.as_bytes());
	}			}

				#[test]
				fn test_relative_to() {
				let path = HgPath::new(b"");
				let base = HgPath::new(b"");
				assert_eq!(Some(path), path.relative_to(base));
				martinvonzUnsubmitted Not Done 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. martinvonz: The argument order here feels backwards. I know some frameworks (like Java's JUnit) put the…
				AlphareAuthorUnsubmitted Done It looks that I took the habit because the test runner in PyCharm that I used when starting with Rust prints `expected` and `actual` in that order. I can reverse it if you really think it's more readable this way. Alphare: It looks that I took the habit because the test runner in PyCharm that I used when starting…

				let path = HgPath::new(b"path");
				let base = HgPath::new(b"");
				assert_eq!(Some(path), path.relative_to(base));

				let path = HgPath::new(b"a");
				let base = HgPath::new(b"b");
				assert_eq!(None, path.relative_to(base));

				let path = HgPath::new(b"a/b");
				let base = HgPath::new(b"a");
				assert_eq!(None, path.relative_to(base));

				let path = HgPath::new(b"a/b");
				let base = HgPath::new(b"a/");
				assert_eq!(Some(HgPath::new(b"b")), path.relative_to(base));

				let path = HgPath::new(b"nested/path/to/b");
				let base = HgPath::new(b"nested/path/");
				assert_eq!(Some(HgPath::new(b"to/b")), path.relative_to(base));

				let path = HgPath::new(b"ends/with/dir/");
				let base = HgPath::new(b"ends/");
				assert_eq!(Some(HgPath::new(b"with/dir/")), path.relative_to(base));
				}
	}			}

This is an archive of the discontinued Mercurial Phabricator instance.

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 18402

rust/hg-core/src/utils/hg_path.rs

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

Revision Contents
Changeset List