Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
bpo-24132: Add direct subclassing of PurePath/Path in pathlib#26906
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Add tests to verify previously untested black-box class properties ofPurePath and Path in pathlib. Collectively, these tests verify all theways the classes represent themselves via isinstance, issubclass,__repr__(), etc. Moreover all class instances spawned by PurePath orPath are also similarly tested.
Add expected failing tests to pathlib that will verify the abilityof PurePath and Path to be subclassed by users. As part of this,test all of the black-box class properties of the newly subclassedPath-like class instances, and the instances generated by their methods.
Existing is_mount method in Path was Posix only and not cross-platform even though it was in a cross-platform base class. The existingdesign relied upon being overriden later in WindowsPath. Consolidatedcode from both into new Path.is_mount() which is now cross-plaformsimiliar to all of the other Path methods.
Replace hardcorded references to pathlib.Path to facilitate makingPath extensible.
Replace factory functionality of PurePath and Path with a psuedo-factorywhich appears to generate it's derived subclasses, but instead justmasquerades the new instance as one of the subclasses. Doing so makesthe resultant versions of PurePath and Path directly subclassable and assuch allows them to pass all of the previously failing subclass tests.
Add tests mirroring existing comparison tests for PurePosixPathand PureWindowsPath but extending them to direct subclasses of PurePath.
Add documentation highlighting the new direct subclass functionality ofPurePath and Path. Also add new is_posix & is_windows doctest conditionsso that system specific doctest code can still be tested instead of justbeing taken as a literal block and always skipped.
Great stuff! Will review in the next couple of days. |
from os import name as system_name | ||
is_posix = system_name == 'posix' | ||
is_windows = system_name == 'nt' | ||
del system_name |
kfollstadJun 25, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or IS_POSIX/IS_WINDOWS? I am unaware of any precedence for the naming convention here. However having the ability to skip tests depending on platform, gives one the ability to actually have sphinx doctest the code in the documentation rather than just skipping it with :: and trusting that it is correct (as is the current practice throughout much of pathlib's documentation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It doesnt really matter in this snippet that’s invisible to readers of the doc 🙂
I think lower case looks fine.
This PR is stale because it has been open for 30 days with no activity. |
class will not generate a differently named class: | ||
.. doctest:: | ||
:pyversion: > 3.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is surprising to me: python docs document one version, so what is this for?
(also > and not >=?)
>>> [Path('/dir').is_absolute, MyPath('/dir').is_absolute()] | ||
[True, True] | ||
>>> [Path().home().drive, MyPath().home().drive] | ||
['', ''] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A minor point: the lists here are a little bit distracting (we are looking at methods/properties, not working with lists).
Could use separate lines, or>>> Path().home().drive, MyPath().home().drive
(which will have parens in the output but that’s fine, we’re used to commas being sometimes multiple things, sometimes a tuple object).
Hey@kfollstad, FYI, I've now got another PR up that makes subclassing possible:#31691 |
Hey@kfollstad, the original bug has now been solved soon I'm going to close this PR. Hope that's OK! Thank you for working on this -- your implementation was helpful while I was working through this problem. |
Uh oh!
There was an error while loading.Please reload this page.
I submit for your consideration a new working version (with documentation) of an extensible, subclassable PurePath/Path to closebpo-24132, a 6-year-old bug. I had previously submitted a PR which added classes to pathlib as a path to solving this, but these commits introduce no new public classes, breaking changes, or deviations from PEP 428.
The crux of the issue here is that Path (and PurePath) are factories which generate their subclasses upon instantiation and as such are actually dependent upon their flavoured subclasses. To get around this, we attach _flavour to PurePath/Path (and anynon PurePosixPath/PureWindowsPath/PosixPath/WindowsPath class) at time of instantiation. Then any subclass will have the essential _flavour attribute available to it. Moreover, in the case that the class being instantiated is PurePath/Path, instead of instantiating another new instance, we just update the class attributes in place so that itbecomes its flavoured subclass. My thanks goes out to Barney Gale for making suggestions that inspired this approach.
In addition, I should mention that@barneygale is working on implementing an AbstractPath interface which will allow for even further extensibility of pathlib.His idea thread on this is here. This was an active consideration as I was writing this, and I specifically chose an implementation which I think works with what he is doing. (Barney, I hope that you find that this to be true - I look forward to your comments.) To verify this, I created a stub version of AbstractPath derived from these commits which I hope further shows the viability of this approach.That code can be found here.
https://bugs.python.org/issue24132