Path improvements
#546
Path improvements
#546
Conversation
|
Draft for now: everything should work but I'd like to know which of these changes would be welcome, if any. Thanks for maintaining this by the way 🙂 |
|
One alternative I'm considering is a |
Kinrany
force-pushed
the
push-qnqozslwvlop
branch
from
44f4967 to
b49dd85
Compare
Kinrany
force-pushed
the
push-qnqozslwvlop
branch
from
b49dd85 to
c6ccb69
Compare
|
Could I have one more CI run please? |
Done |
src/path/mod.rs
Outdated
| assert_eq!(Path::ROOT.parts().count(), Path::ROOT.parts_count()); | ||
|
|
||
| let path = path("foo/bar/baz"); | ||
| assert_eq!(path.parts().count(), path.parts_count()); |
There was a problem hiding this comment.
could you also please add an assertion that the count is 3?
|
|
||
| Self { raw } | ||
| impl<'a, I: Into<PathPart<'a>>> Extend<I> for Path { | ||
| fn extend<T: IntoIterator<Item = I>>(&mut self, iter: T) { |
There was a problem hiding this comment.
could you please add tests (perhaps doctests) using this feature explicitly to help others find it?
There was a problem hiding this comment.
Hmm, note that I haven't added any docs on Path directly. Since this is a standard trait, I expect people to find it by looking at the list of implemented traits 🤔
src/path/mod.rs
Outdated
| if s.raw.is_empty() { | ||
| continue; | ||
| } | ||
| self.raw.push(DELIMITER_CHAR); |
There was a problem hiding this comment.
Can you add some tests showing how this works for:
- Empty iterators
- Iterator with a single element
- Iterator with multiple elements
- Extending an existing (non empty) Path? (it looks like maybe this will add an extra
/🤔 )
|
CI appears to show some regressions |
|
Thank you, will address! |
|
marking as draft while CI issues are resolved |
|
please mark it ready for review when it is ready for another look |
|
We are preparing the next release in a few days so if we want to include this one we need to get it ready to go |
|
I was going to say I'll probably do it this weekend. But, oh look, I only meant to see how much I'll need to change and now it's done. Haha. I added |
|
Sounds reasonable (but I haven't thought about it deeply yet) |
|
Is there a way to run the same checks that run in CI locally? Should I do anything other than When tests run, a few items end up marked as unused. I assume that's fine. Or should I fix that in main in a separate PR first? Could be a toolchain update thing. |
You can look at the output of the CI runs and see what commands it is running (they are all listed in various yml files in |
|
That should do it. All the new stuff has tests directly covering it now. |
|
Thank you! |
3 participants
Which issue does this PR close?
Closes #545
Rationale for this change
fn prefix(&self)was the first thing I really wanted to have onPath, as opposed to copying from project to project.It also benefits from direct access to
raw. In userland we have to usepathsand collect, and omitting the last item is not trivial.What changes are included in this PR?
Implementations of everything listed with minimal docs and tests.
Are there any user-facing changes?
Most of these changes are user-facing.
Internal changes:
impl FromIteratordelegates most of the logic toimpl ExpandPath::partsnow returns a custom iterator typeDELIMITER_CHARThere should be no breaking changes.