Implemented DoubledEndedIterator for Ancestors<'_> + added tests and updated doc comments#153239
Open
asder8215 wants to merge 1 commit intorust-lang:mainfrom
Open
Implemented DoubledEndedIterator for Ancestors<'_> + added tests and updated doc comments#153239asder8215 wants to merge 1 commit intorust-lang:mainfrom
asder8215 wants to merge 1 commit intorust-lang:mainfrom
Conversation
Collaborator
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
79234be to
eb5b1fc
Compare
This comment has been minimized.
This comment has been minimized.
eb5b1fc to
26917f2
Compare
This comment has been minimized.
This comment has been minimized.
…updated doc comments
26917f2 to
b436c61
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR revolves around the tracking issue for implementing
DoubleEndedIteratorforAncestors<'_>.I tried to minimize the information needed in
Ancestors<'_>struct to effectively produce the correct backward and forward paths using a front and back index; I did opt to use a&'a [u8]over&'a Pathbecause it would reduce code in.next()/.next_back()where I wouldn't need to constantly convert the Path into a u8 slice. Due to how relative paths worked previously forAncestors<'_>, I had to introduce logic that allowed for returning an empty string at the start/end of a relative path for.next_back()/.next(); I'm open to a lot of feedback on how to make the code neater to address these cases.I tested this pretty extensively on my Linux machine (comparing it with the output of what
Ancestors<'_>previously returned); I do have to do more testing for Windows (been struggling a bit to get my Windows VM to run my shared folder containing the rust project through./xthough, so I may need some help on this), which there are some windows-specific test intests/path_ancestors.rs. As far as I'm aware as well, the create_dir_all tests, which does useAncestors<'_>underneath the hood, all passes.I am aware of one breaking change in this code though. I realized for a path like:
Its
Ancestor<'_>iterator contains:The previous implementation doesn't strip trailing slashes for the first path, but in subsequent paths it does eliminate trailing slashes. You can see this here.
With how I use ascii separators to determine that we've reached the next component, this does make it a bit difficult to find a neat way to handle this logic (and do this symmetrically as well for
.next_back()). For example, consider the two paths:With how I'm using a back index approach, I save what the current back idx is in a variable (curr_back), then iterate through the u8 slice until I hit an ascii separator, advance more through potential ascii separators, and then stop reading from there if there are no more adjacent ascii separators, returning what
path[..curr_back]is (trimming separators). Doing this on the first path would return "/foo/bar/baz" in the first call (since I trim separators, but otherwise this would be "/foo/bar/baz/") and then return "/foo/bar/baz" again in the next call (this is because curr_back is set to the idx of where the last "/" is at in "/foo/bar/baz/"). Doing this for the second path would return "/foo/bar/baz" in the first call and then "/foo/bar" in the next call. I currently just trim separators from the original path before giving that to theAncestors<'_>struct when.ancestors()is called.I'm hoping this change isn't room for big concerns because removing trailing separators for the original path does not break logic in accessing that path. However, if someone were to use
Ancestors<'_>for printing paths recursively up, this would cause changes to the first path, which I do not see it being a huge issue if they want to print the original path unchanged since they can opt to consume the first item in the iterator and print the original path. If I do, however, need to account for this logic, then I will try and see what I can do; I'm open to feedback in handling this case as well.