Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add std::path::Path::ancestors #48420

Merged
merged 1 commit into from
Mar 1, 2018
Merged

Add std::path::Path::ancestors #48420

merged 1 commit into from
Mar 1, 2018

Conversation

teiesti
Copy link
Contributor

@teiesti teiesti commented Feb 22, 2018

Tracking issue: #48581

This pull request adds a method called parents ancestors to std::path::Path. parents ancestors is a generalized version of parent. While parent returns the path without its final component, parents ancestors returns an iterator that yields every ancestor. This includes the path itself, the parent, the grandparent and so on. This is done by repeatedly calling parent. In case parent returns None, the iterator returned by parents ancestors does likewise.

Why is such a method useful?
A method like parents ancestors is especially useful, if a task has to be done for the current directory and every directory up the tree. (Think of Cargo searching for the Cargo.toml!)

Why does the iterator yield the original path instead of starting with the parent?
It is easier to skip the original path with .skip(1) than it is to add the original path if necessary.

Why is the method called parents instead of ancestors?
When sorted , parents comes right after parent. It is therefore grouped with its more specific counterpart within the documentation.

Why is the method called ancestors instead of parents?

  1. As @alexcrichton pointed out, Cargo has a similar method called ancestors.
  2. As @sfackler pointed out, it might be weird that an iterator called Ancestor yields the current path. It is more reasonable that the current path is a "null ancestor" than a "null parent".
  3. As @zackmdavis pointed out, the name parents might be misleading.

Two more notes:

  1. Since this is my first contribution to the standard library, I am actually not sure if an RFC is required!
  2. Since I am not a native speaker, someone should double-check my English!

@nagisa nagisa added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Feb 22, 2018
@nagisa
Copy link
Member

nagisa commented Feb 22, 2018

This hasn’t been assigned a reviewer, so assigning first person from T-libs, I can think of

r? @BurntSushi

@pietroalbini pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 22, 2018
/// The iterator will yield the [Path] that is returned if the [`parent`] method is used zero
/// or more times. That means, the iterator will yield `&self`, `&self.parent().unwrap()`,
/// `&self.parent().unwrap().parent().unwrap()` and so on. If the [`parent`] method returns
/// [`None`], the iterator will do likewise.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be useful to explicitly clarify here that the iterator returned will always yield at least one value.

@BurntSushi
Copy link
Member

I do agree with the utility of this function, and I've certainly had to write this iterator out by hand before.

So I think this seems fine to me, but let's see if anyone has any objections before merging. cc @rust-lang/libs

/// [`parents`]: struct.Path.html#method.parents
/// [`Path`]: struct.Path.html
#[derive(Copy, Clone, Debug)]
#[unstable(feature = "path_parents", issue = "0")]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line raises two open questions for me:

  1. Is the feature name appropriate?
  2. What issue number can I use?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature name seems OK to me, as long as it is distinct from any other feature name. :-)

I believe that if we decide to merge this, you need to create a tracking issue for stabilizing this feature, and that's the number that goes here.

I also think there is some docs where features are documented. But I don't know where it is.

@alexcrichton
Copy link
Member

Sounds like a great idea to me! We've actually got basically this method already in Cargo

@sfackler
Copy link
Member

The name parents seems a bit weird given that the path itself is included, but I agree that behavior seems reasonable. I can't really think of a better name though :(

@zackmdavis
Copy link
Member

When sorted , parents comes right after parent. It is therefore grouped with its more specific counterpart within the documentation.

This seems like a weak justification for choosing parents over ancestors. (Before reading the description, I expected parents to somehow reverse-follow hardlinks to yield all immediate-parent directories that linked the same file. I also have an easier time accepting the path itself as the "null ancestor" than as the "null parent".) If we just want someone reading the documentation of one method to be made aware of the other, a "See also .parents()" link should suffice.

@teiesti
Copy link
Contributor Author

teiesti commented Feb 27, 2018

I like these arguments. I will prepare a commit that changes the name to ancestors.

@teiesti teiesti changed the title Add std::path::Path::parents Add std::path::Path::ancestors Feb 27, 2018
@eav
Copy link

eav commented Feb 28, 2018

maybe paths() ?

@teiesti
Copy link
Contributor Author

teiesti commented Feb 28, 2018

I think, paths is a little bit too generic. Someone could believe that a methods called paths creates an iterator that descends through all the paths in a directory just like the walkdir crate does.

@eav
Copy link

eav commented Feb 28, 2018

well, I think the name should be more or less generic, because it lists all paths, documentation should clarify the meaning though, another option segments

@teiesti
Copy link
Contributor Author

teiesti commented Feb 28, 2018

well, I think the name should be more or less generic, because it lists all paths

The method does not list all paths at all. It lists all paths that are ancestors of self. This might sound like a small distinction but it is actually crucial. Consider the following directory structure:

/
└── grandparent
    ├── aunt
    └── parent
        ├── self
        │   ├── child1
        │   │   └── grandchild
        │   └── child2
        └── silbling

Calling ancestors on self would lead to

/grandparent/parent/self
/grandparent/parent
/grandparent
/

while all paths would (in my understanding of the term) be something like

/
/grandparent
/grandparent/parent
/grandparent/parent/self
/grandparent/parent/self/child1
/grandparent/parent/self/child1/grandchild
/grandparent/parent/self/child2
/grandparent/parent/silbling
/grandparent/aunt

another option segments

In my opinion, something like parent/self would also be a segment.

@eav I really appreciate your ideas! But what is so wrong with ancestors?

@BurntSushi
Copy link
Member

Either ancestors or parents seem suitable to me. I'm opposed to paths. Let's move on from it. The name can be further debated during stabilization.

@BurntSushi
Copy link
Member

@teiesti Could you squash this down to one commit? Then I think this should be good to go!

Squashed commit of the following:

commit 1b5d55e26f667b1a25c83c5db0cbb072013a5122
Author: Tobias Stolzmann <[email protected]>
Date:   Wed Feb 28 00:06:15 2018 +0100

    Bugfix

commit 4265c2db0b0aaa66fdeace5d329665fd2d13903a
Author: Tobias Stolzmann <[email protected]>
Date:   Tue Feb 27 22:59:12 2018 +0100

    Rename std::path::Path::parents into std::path::Path::ancestors

commit 2548e4b14d377d20adad0f08304a0dd6f8e48e23
Author: Tobias Stolzmann <[email protected]>
Date:   Tue Feb 27 12:50:37 2018 +0100

    Add tracking issue

commit 3e2ce51a6eea0e39af05849f76dd2cefd5035e86
Author: Tobias Stolzmann <[email protected]>
Date:   Mon Feb 26 15:05:15 2018 +0100

    impl FusedIterator for Parents

commit a7e096420809740311e19d963d4aba6df77be2f9
Author: Tobias Stolzmann <[email protected]>
Date:   Mon Feb 26 14:38:41 2018 +0100

    Clarify that the iterator returned will yield at least one value

commit 796a36ea203cd197cc4c810eebd21c7e3433e6f1
Author: Tobias Stolzmann <[email protected]>
Date:   Thu Feb 22 14:01:21 2018 +0100

    Fix examples

commit e279383b21f11c97269cb355a5b2a0ecdb65bb0c
Author: Tobias Stolzmann <[email protected]>
Date:   Thu Feb 22 04:47:24 2018 +0100

    Add std::path::Path::parents
@teiesti
Copy link
Contributor Author

teiesti commented Feb 28, 2018

@BurntSushi Done.

@eav
Copy link

eav commented Feb 28, 2018

for me parents or ancestors sound like they return paths without self

/grandparent/parent
/grandparent
/

@BurntSushi
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 28, 2018

📌 Commit b9e9b4a has been approved by BurntSushi

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2018
@BurntSushi
Copy link
Member

@teiesti Thanks!

@eav But neither paths nor segments are clearly better. The concept of "something is its own parent" is common enough that it's not an entirely arcane meaning, and in the absence of a better name, it seems appropriate. Let's please move naming discussion to the tracking issue.

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 28, 2018
bors added a commit that referenced this pull request Feb 28, 2018
Rollup of 10 pull requests

- Successful merges: #48355, #48359, #48380, #48419, #48420, #48461, #48522, #48570, #48572, #48603
- Failed merges:
@bors bors merged commit b9e9b4a into rust-lang:master Mar 1, 2018
@teiesti teiesti deleted the path_parents branch March 1, 2018 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants