8000 bpo-21041: Add negative indexing to pathlib path parents by ypankovych · Pull Request #21799 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content
Dismiss alert

bpo-21041: Add negative indexing to pathlib path parents #21799

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 23, 2020
Merged

bpo-21041: Add negative indexing to pathlib path parents #21799

merged 1 commit into from
Nov 23, 2020

Conversation

ypankovych
Copy link
Contributor
@ypankovych ypankovych commented Aug 9, 2020 •
edited
Loading

As I can see, pathlib path parents don't support negative indexing:

>>> import pathlib
>>> path = pathlib.PosixPath("some/very/long/path/here")
>>> path.parents[-1]
...
    raise IndexError(idx)
IndexError: -1

That's kinda weird for python. I mean, in regular list/etc if I need the last element, I'd normally do list[-1], but here to get the last parent, I need to actually know how many parents do I have.

So now, I can do something like this:

>>> parents_count = len(path.parents) - 1
>>> path.parents[parents_count]
PosixPath('.')

So that's how I can get the last parent. But is it pythonic? No.

So, I decided to fix this, and now we can do negative indexing:

>>> path.parents[-1] == path.parents[parents_count]
True
>>> path.parents[-2] == path.parents[parents_count - 1]
True

https://bugs.python.org/issue21041

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@ypankovych

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@ypankovych ypankovych changed the title bpo-41511: Add negative slicing to pathlib path parents bpo-21041: Add negative slicing to pathlib path parents Aug 10, 2020
@thejcannon
Copy link
Contributor

Minor: The title says "negative slicing", however what this change is fixing is negative indexing.

@ypankovych ypankovych changed the title bpo-21041: Add negative slicing to pathlib path parents bpo-21041: Add negative indexing. to pathlib path parents Aug 10, 2020
@ypankovych ypankovych changed the title bpo-21041: Add negative indexing. to pathlib path parents bpo-21041: Add negative indexing to pathlib path parents Aug 10, 2020
@ypankovych
8000 Copy link
Contributor Author

Minor: The title says "negative slicing", however what this change is fixing is negative indexing.

Thanks, fixed

@remilapeyre
Copy link
Contributor

Hi @ypankovych, thanks for writing this PR. It's not sure this change will be accepted but it will require a NEWS entry, you can add one by using https://blurb-it.herokuapp.com/

@ypankovych
Copy link
Contributor Author

@remilapeyre thanks, done.

remilapeyre
remilapeyre approved these changes Aug 11, 2020
View reviewed changes
Copy link
Contributor
@remilapeyre remilapeyre left a comment

Choose a reason for hiding this comment

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

Whether or not this is the wanted behaviour still needs to be discussed on the bpo issue, but the implementation looks correct to me. Thanks @ypankovych !

@ypankovych
Copy link
Contributor Author

@remilapeyre thanks for your review.

srinivasreddy
srinivasreddy reviewed Aug 13, 2020
View reviewed changes
pganssle
pganssle requested changes Nov 19, 2020
View reviewed changes
Copy link
Member
@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

@ypankovych Since we're changing the semantics of this, could you please add a .. versionchanged:: 3.10 annotation to the pathlib documentation, and add a "What's New" entry for Python 3.10?

Other than that, this looks good to me. Given the lack of explicit objections (Chesterton's fence aside) and the fact that this has been stalled on no one taking decisive action for the past 6 years or so, I'm inclined to go ahead and merge this once it has a What's New entry.

@pitrou As the original author of pathlib, do you have any objections to that course of action?

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@pitrou
Copy link
Member
pitrou commented Nov 19, 2020

I have no objection to this :-) I'm not sure why I didn't add support for negative indexing, but it's probably laziness.

@thejcannon thejcannon mentioned this pull request Nov 19, 2020
Merged
@pganssle
Copy link
Member

@ypankovych I have merged #11165, which introduced some merge conflicts with this PR. Can you re-base against the master branch and resolve the conflicts?

You'll also want to modify the .. versionchanged:: 3.10 directive in the documentation to say something like "The parents sequence now supports a :term:slice or negative index values."

@ypankovych
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@pganssle: please review the changes made to this pull request.

pganssle
pganssle approved these changes Nov 23, 2020
View reviewed changes
@ypankovych @pganssle
Added support for negative indexes to PurePath.parents
0138810 
This commit also fixes up some of the overlapping documentation changed
in bpo-35498, which added support for indexing with slices.

Fixes bpo-21041.
https://bugs.python.org/issue21041

Co-authored-by: Paul Ganssle <p.ganssle@gmail.com>
Co-authored-by: Rémi Lapeyre <remi.lapeyre@henki.fr>
@pganssle pganssle force-pushed the bugfix/41511-pathlib-parents-negative-slicing branch from cfbf0f6 to 0138810 Compare November 23, 2020 15:50
@pganssle pganssle closed this Nov 23, 2020
@pganssle pganssle reopened this Nov 23, 2020
@pganssle pganssle merged commit 79d2e62 into python:master Nov 23, 2020
@ypankovych ypankovych deleted the bugfix/41511-pathlib-parents-negative-slicing branch November 23, 2020 21:31
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@ypankovych @pganssle @adorilson
Added support for negative indexes to PurePath.parents (pythonGH-21799)
df7ed6b 
This commit also fixes up some of the overlapping documentation changed
in bpo-35498, which added support for indexing with slices.

Fixes bpo-21041.
https://bugs.python.org/issue21041

Co-authored-by: Paul Ganssle <p.ganssle@gmail.com>
Co-authored-by: Rémi Lapeyre <remi.lapeyre@henki.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srinivasreddy srinivasreddy srinivasreddy left review comments

@pganssle pganssle pganssle approved these changes

@remilapeyre remilapeyre remilapeyre approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ypankovych @the-knights-who-say-ni @thejcannon @remilapeyre @bedevere-bot @pitrou @pganssle @srinivasreddy
0