gh-68320, gh-88302 - Allow for `pathlib.Path` subclassing

[barneygale]

Users may wish to define subclasses of pathlib.PurePath and 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

• Issue: gh-68320

[barneygale]

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

[brettcannon]

@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.

[bedevere-bot]

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]

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?

[barneygale]

I didn't expect the Spanish Inquisition!

[bedevere-bot]

Nobody expects the Spanish Inquisition!

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

[barneygale]

Hey @brettcannon! I've just merged main, including #91882, into this branch. As a result, most of the iffy code you identified in your previous review no longer exists! Could you review again please? Thanks v much

[brettcannon]

@barneygale Right now I'm focusing on bugfix PRs for 3.11, but as soon as that queue is cleared out I will be able to get to this feature PR.

[bedevere-bot]

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]

Thank you @brettcannon, much appreciated.

A clarification: I'm not intending for users to implement their own _flavour, as I expect posixpath and ntpath to cover 99.99% of cases. The flavour is there to provide the very lowest level of path manipulation for use in PurePath / AbstractPath / Path methods. The path classes themselves are the user API, not the flavours. As such, I don't think the self._flavour is posixpath checks break anything important.

That said, I don't much like the is posixpath checks. I'll respond to each case inline.

[barneygale]

I think #95450 needs to be merged before I can fix the Windows tests, so I'm marking this PR as a draft.

[brettcannon]

The flavour is there to provide the very lowest level of path manipulation for use in PurePath / AbstractPath / Path methods.

So what is the expectation around AbstractPath and this? How is e.g. a ZipPath to make sure it's using posixpath as its flavour? Or am I getting ahead of myself here and that comes in a later PR? And thus "subclassing" here is just to allow potential overriding of the public methods as they are and a/the future AbstractPath will handle fully abstract implementation?

[barneygale]

The flavour is there to provide the very lowest level of path manipulation for use in PurePath / AbstractPath / Path methods.

So what is the expectation around AbstractPath and this? How is e.g. a ZipPath to make sure it's using posixpath as its flavour? Or am I getting ahead of myself here and that comes in a later PR? And thus "subclassing" here is just to allow potential overriding of the public methods as they are and a/the future AbstractPath will handle fully abstract implementation?

Yep spot on. After this PR, users can subclass from PurePosixPath or PureWindowsPath if they need a specific flavour, or they can subclass from PurePath or Path if they need the current OS's flavour. So all use cases are satisfied. When we introduce AbstractPath, I think we'd remove the underscore prefix from _flavour and document it. Thus users would write:

import posixpath
import pathlib

class ZipPath(pathlib.AbstractPath):
    flavour = posixpath

... but that's a little ways away still.

[barneygale]

An alternative to making AbstractPath.flavour public would be to add AbstractPosixPath and AbstractWindowsPath classes. But I'd rather not go there for the sake of one attribute! :)

[barneygale]

I didn't expect the spanish inquisition!

[bedevere-bot]

Nobody expects the Spanish Inquisition!

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

[barneygale]

I've removed all flavour is posixpath and flavour is ntpath checks, except for these:

• ~~_split_root() - these are only for performance. I'm planning to move the implementation into ntpath and posixpath, but I'd like to massage pathlib a little more first.~~ edit: found a better way to do it :)
• is_absolute() - unavoidable due to longstanding incompatible ntpath.isabs() behaviour.

[brettcannon]

Are the Windows failures in relation to https://github.com/python/cpython/issues/96290 ?

[barneygale]

Are the Windows failures in relation to #96290 ?

Yes

[bedevere-bot]

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]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

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

[barneygale]

Hey @brettcannon, if you have a chance to review again I'd appreciate it! I've removed the tests affected by the change to normpath(), as, per discussion on that bug, the new behaviour is reasonable. If there's something I can do to make this easier to review (e.g. split some aspects of it into another PR?) I'd be happy to do so!

[barneygale]

Hey @brettcannon, anything else needed here do you think? Thank you!

[brettcannon]

@barneygale not at this point. I was on vacation for 3 weeks and it's taken a week to catch up; today looks like my last day of digging myself out from emails, GH notifications, etc.

Don't worry, this is effectively at the top of my non-work review queue. 🙂

[barneygale]

Rather than fix conflicts in the growing relative_to() method, I've opened a PR here that makes it call through to os.path.relpath(): #99031

I'll mark this as a draft until the above lands.

[bedevere-bot]

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]

I have made the requested changes; please review again!

Turns out the conflicts were pretty simple to fix :)

[bedevere-bot]

Thanks for making the requested changes!

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

[brettcannon]

@barneygale now that #99031 is merged, let me know when you're ready for me to review this again.