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

Jump to bottom

gh-68320, gh-88302 - Fix pathlib.Path subclassing. #31691

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

gh-68320, gh-88302 - Fix pathlib.Path subclassing. #31691

wants to merge 17 commits into from
+321 −388

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented Mar 5, 2022 •
edited

Users may wish to define subclasses of pathlib.Path to add or modify existing methods. Before this change, attempting to instantiate a subclass raised an exception like:

AttributeError: type object 'PPath' has no attribute '_flavour'

Previously the _flavour attribute was assigned as follows:

PurePath._flavour        = xxx not set!! xxx
PurePosixPath._flavour   = _PosixFlavour()
PureWindowsPath._flavour = _WindowsFlavour()

It's now set as follows:

PurePath._flavour        = os.path
PurePosixPath._flavour   = posixpath
PureWindowsPath._flavour = ntpath

Functionality from _PosixFlavour and _WindowsFlavour is moved into PurePath as underscored-prefixed classmethods. Flavour classes are removed.

A deeper dive into this patch can be read here: https://discuss.python.org/t/make-pathlib-extensible/3428/42

Fixes #68320 #88302

@barneygale
bpo-24132, bpo-44136 - Fix pathlib.Path subclassing.
7371220 
Users may wish to define subclasses of `pathlib.Path` to add or modify
existing methods. Before this change, attempting to instantiate a subclass
raised an exception like:

    AttributeError: type object 'PPath' has no attribute '_flavour'

Previously the `_flavour` attribute was assigned as follows:

    PurePath._flavour        = xxx not set!! xxx
    PurePosixPath._flavour   = _PosixFlavour()
    PureWindowsPath._flavour = _WindowsFlavour()

This commit replaces it with a `_pathmod` attribute, set as follows:

    PurePath._pathmod        = os.path
    PurePosixPath._pathmod   = posixpath
    PureWindowsPath._pathmod = ntpath

Functionality from `_PosixFlavour` and `_WindowsFlavour` is moved into
`PurePath` as underscored-prefixed classmethods. Flavours are removed.
@barneygale barneygale mentioned this pull request Mar 5, 2022
Draft
merwok
merwok reviewed Mar 5, 2022
View changes
Lib/pathlib.py Show resolved Hide resolved
@brettcannon brettcannon self-requested a review Mar 7, 2022
@barneygale
Copy link
Contributor Author

@barneygale barneygale commented Apr 6, 2022

Hey @brettcannon, just bumping this PR in case it dropped off your radar.

@brettcannon
Copy link
Member

@brettcannon brettcannon commented Apr 7, 2022

@barneygale it hasn't, but my PR review queue is 11 PRs deep, this is in position 7, and I have worry about getting PEP 594 done in time for 3.11b1. Plus I have some stuff I need to get done for work this month which is eating into my paid OSS time.

@barneygale barneygale mentioned this pull request Apr 15, 2022
Closed
@barneygale barneygale changed the title bpo-24132, bpo-44136 - Fix pathlib.Path subclassing. gh-68320, gh-88302 - Fix pathlib.Path subclassing. Apr 24, 2022
@brettcannon brettcannon reopened this Apr 27, 2022
@barneygale barneygale closed this Apr 29, 2022
@barneygale barneygale reopened this Apr 29, 2022
brettcannon
brettcannon requested changes Apr 29, 2022
View changes
Copy link
Member

@brettcannon brettcannon left a comment

Most of the comments are either code touch-ups or asking to make the code more self-describing.

Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Show resolved Hide resolved
Lib/pathlib.py Show resolved Hide resolved
Lib/pathlib.py Show resolved Hide resolved
Lib/pathlib.py Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 29, 2022

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.

barneygale and others added 2 commits Apr 29, 2022
@barneygale
Copy link
Contributor Author

@barneygale barneygale commented Apr 29, 2022

Thanks @brettcannon, working to address your feedback now.

Some of your comments relate to the _splitroot() and _split_extended_path() methods. To be clear: these have been moved from the _WindowsFlavour and _PosixFlavour classes, which are removed in this patch. They have not been written afresh.

I'm happy to fix up formatting and whatnot, but I don't want to modify the implementations of those methods too much here. It adds undue risk to the overall PR as it's possible we'll change behaviour without realising it.

And in this particular case, the deficiencies in _splitroot() etc are being addressed elsewhere - see #91882.

Does that go some of the way towards explaining why I'm mostly leaving these implementations alone, aside moving them from the "flavour" classes into PurePath?

merwok
merwok reviewed Apr 30, 2022
View changes
Lib/pathlib.py Show resolved Hide resolved
gst
gst reviewed Apr 30, 2022
View changes
Lib/pathlib.py Show resolved Hide resolved
@barneygale
Copy link
Contributor Author

@barneygale barneygale commented May 3, 2022

I didn't expect the Spanish Inquisition!

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 3, 2022

Nobody expects the Spanish Inquisition!

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

@bedevere-bot bedevere-bot requested a review from brettcannon May 3, 2022
AlexWaygood
AlexWaygood reviewed May 3, 2022
View changes
Lib/pathlib.py Outdated Show resolved Hide resolved
@barneygale @AlexWaygood
Update Lib/pathlib.py
3624c97 
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@barneygale barneygale mentioned this pull request May 20, 2022
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gst gst

@merwok merwok

@eryksun eryksun

@arhadthedev arhadthedev

@AlexWaygood AlexWaygood

@brettcannon brettcannon

Assignees
No one assigned
Labels
awaiting change review CLA signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Direct sub-classing of pathlib.Path
9 participants
@barneygale @brettcannon @bedevere-bot @gst @merwok @eryksun @arhadthedev @AlexWaygood @the-knights-who-say-ni