This revision now requires changes to proceed.
kevincox added inline comments.
kevincox requested changes to this revision.
INLINE COMMENTS
> hg_path.rs:131
> + let base = base.as_ref();
> + if base.len() == 0 {
> + return Some(self);
`base.is_empty()`? If we don't have that helper we should probably add it.
> hg_path.rs:131
> + let base = base.as_ref();
> + if base.len() == 0 {
> + return Some(self);
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?
> hg_path.rs:134
> + }
> + let is_dir = base.len() > 0 && base.as_bytes().last() == Some(&b'/');
> + if is_dir && self.starts_with(base) {
Instead of using `.last()` can we add and use `base.ends_with(b'/')`?
> hg_path.rs:134
> + }
> + let is_dir = base.len() > 0 && base.as_bytes().last() == Some(&b'/');
> + if is_dir && self.starts_with(base) {
`base.len() > 0` is redundant both with the early return above and because
`last()` will return `None` if `base` is empty.
> hg_path.rs:215
> + fn eq(&self, other: &HgPath) -> bool {
> + self.inner == other.as_bytes()
> + }
I would do `self.as_bytes()` for consistency.
Or even better I believe you can `#[derive(Eq,PartialEq)]`.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7526/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7526
To: Alphare, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
