Skip to content

bpo-24132: Direct sub-classing of pathlib.Path #26438

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

Closed
wants to merge 11 commits into from

Conversation

kfollstad
Copy link
Contributor

@kfollstad kfollstad commented May 28, 2021

Updated based on feedback - currently marked as draft

I submit for your consideration what I believe is the first working version (with documentation) of an extensible, subclassable PurePath/Path to close bpo-24132, a 6-year-old bug. I have also posted a more detailed version of this explanation on discuss.python.org because this fix involves adding classes to pathlib and understanding why that is required takes a significant explanation.

These classes are a complete set of alternatives to PurePath and Path which omit the factory design but instead attach the flavour to the class at the time of instantiation. These newly designed classes still pass all of the test cases that are run against PurePath and Path on both Windows and Linux. Because they omit the factory design, they don't have inverted dependencies and therefore are naively subclassable and extensible in any way you see fit.

To this end I give you:

SimplePath (subclassable alternative to PurePath)
SimplePosixPath (subclassable alternative to PurePosixPath)
SimpleWindowsPath (subclassable alternative to PureWindowsPath)
FilePath (subclassable alternative to Path/PosixPath/WindowsPath)

On Windows, SimplePath behaves as if it were PureWindowsPath. On Posix, it behaves as PurePosixPath. Similarly, FilePath behaves like WindowsPath on Windows and PosixPath on Posix. These four classes combined could, if one were so inclined, act as a complete replacement for the existing six Path/PurePath classes.

Despite being 11 commits, it's essentially just two refactors. The first splits the two responsibilities PurePath has, separating the base class methods into _PurePathBase. The second does the same with Path, moving the base class methods into PathIOMixin. The other commits are just minor tweaks and documentation to account for all of that.

https://bugs.python.org/issue24132

kfollstad added 11 commits May 28, 2021 14:14
Introduce stub classes with initial tests on what will become
subclassable alternatives to PurePath/Path that do not have
inverted dependencies.
Replace references in PurePath and changed function in
_BasePurePathTest.test_ordering_common to test missing execution paths.
This is preparation for seperating the base class and factory
functionality in PurePath in order to fully implement SimplePath.
Introduce new base class _PurePathBase and move all of the methods
from PurePath into it (except for the factory __new__) for use in
building SimplePath / FilePath.
SimplePath provides a subclassable alternative to PurePath which is
otherwise functionally equivalent -- passing all of the existing
PurePath tests.
Partial implementation of FilePath which offers all of the PurePath
and SimplePath functionality and passes the equivalent existing tests.
Existing is_mount method in Path was Posix only and not cross
platform even though it was in a cross-platform base class. The existing
design relied upon being overriden later in WindowsPath. Consolidated
code from both into new Path.is_mount() which is now cross plaform
similiar to all of the other Path methods.
Replace hardcorded references to pathlib.Path so that the base
class and factory functionality of Path can be seperated.
FilePath provides a subclassable alternative to Path which is
otherwise functionally equivalent -- passing all of the existing
Path tests.
SimplePosixPath and SimpleWindowsPath provide a subclassable
alternative to PurePosixPath and PureWindowsPath which are otherwise
functionally equivalent -- passing all of the existing PurePosixPath
and PureWindowsPath tests.
Fix _BasePurePath comparison operators so that all of the new
path classes in pathlib are comparable with one another and return
results that are consistent with the existing behavoir, i.e.
Path('/') == PurePath('/').
Document and make public FilePath, SimplePath, SimplePosixPath,
SimpleWindowsPath, and PathIOMixin. Also add section to pathlib
documentation regarding the limitations of PurePath and Path with
regards to subclassing and how to use these new classes as alternatives.
@barneygale
Copy link
Contributor

On Windows, SimplePath behaves as if it were PureWindowsPath. On Posix, it behaves as PurePosixPath. Similarly, FilePath behaves like WindowsPath on Windows and PosixPath on Posix. These four classes combined (less PathIOMixin) could, if one were so inclined, act as a complete replacement for the existing six Path/PurePath classes.

This is a key design decision - should we depart from pathlib's existing Path, WindowsPath and PosixPath abstraction and handle both systems in a single type?

My personal view is that this is just too large a departure from how pathlib works right now. It's too hard to mentally model and explain. You and I only understand it because we've had our noses in pathlib for too long! If I could go back in time and convince Antoine to consider using Path for all kinds of concrete path I would, that ship has already sailed in my view.

I don't think this needs 5 new classes to implement. We have options for how to make creating a trio of MyPath, MyPosixPath and MyWindowsPath pretty painless.

I also have an incurable phobia of mixin classes :(

@kfollstad kfollstad marked this pull request as draft May 31, 2021 05:01
@kfollstad
Copy link
Contributor Author

kfollstad commented May 31, 2021

@barneygale, thank you very much for taking the time to review my PR / ideas thread. I know you have spent a lot of time wrestling with pathlib which makes your input all the more valuable. I've marked this draft while address some of the points you made.

@kfollstad
Copy link
Contributor Author

kfollstad commented Jun 25, 2021

Closing this and replacing it with an alternative approach to solving bpo-24132 via #26906 which makes no changes to the API.

@kfollstad kfollstad closed this Jun 25, 2021
@projetmbc projetmbc mannequin mentioned this pull request Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants