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

GH-89812: Add pathlib._PurePathExt #104810

Closed
wants to merge 20 commits into from

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented May 23, 2023

Move __fspath__(), __bytes__() and as_uri() methods from PurePath to a new _PurePathExt subclass. This new subclass is inherited by PurePosixPath, PureWindowsPath and Path.

Because PurePath isn't directly instantiatable (you get a PurePosixPath or PureWindowsPath instance back), this shouldn't change user-facing behaviour.

The methods must not be inherited future tarfile.TarPath and pathlib.AbstractPath classes, which will subclass PurePath.

This internal class excludes the `__fspath__()`, `__bytes__()` and
`as_uri()` methods, which must not be inherited by a future
`tarfile.TarPath` class.
@barneygale barneygale changed the title GH-89812: Add pathlib._LexicalPath GH-89812: Add pathlib._BasePurePath May 23, 2023
@barneygale
Copy link
Contributor Author

The test naming is pretty janky. #104829 will help.

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 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
barneygale and others added 2 commits May 24, 2023 16:04
Co-authored-by: Éric <merwok@netwok.org>
Lib/pathlib.py Outdated Show resolved Hide resolved
barneygale and others added 2 commits May 24, 2023 17:36
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Lib/os.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
@barneygale
Copy link
Contributor Author

Would anyone be willing to review #104829? It blocks this PR. (please and thank you!)

@barneygale barneygale marked this pull request as draft June 7, 2023 20:26
@barneygale barneygale marked this pull request as ready for review June 14, 2023 16:12
@barneygale
Copy link
Contributor Author

#104829 has landed, so test naming should be much improved! Think this is ready for review again.

@barneygale barneygale changed the title GH-89812: Add pathlib._BasePurePath GH-89812: Add pathlib._PurePathExt Jun 22, 2023
@barneygale
Copy link
Contributor Author

barneygale commented Jun 22, 2023

I've revised the implementation such that the methods are actually removed from PurePath. This will allow our future AbstractPath class to inherit PurePath, as it should conceptually:

image

(Contrast this with the earlier proposed hierarchy where PurePath and AbstractPath are siblings.

The methods are moved to a new _PurePathExt class, which is inherited by PurePosixPath, PureWindowsPath and Path.

This change will not be visible to users who try to instantiate PurePath, because they'll always get a PurePosixPath or PureWindowsPath instance back.

It will be visible to users who subclass PurePath, but that has only just become possible in 3.12. As such I think it's worth backporting this into 3.12 for consistency.

@barneygale
Copy link
Contributor Author

barneygale commented Jun 22, 2023

Here's a version of the diagram that includes the private _PurePathExt class. The diagram in the post above excluded it in order to show the public view of the class hierarchy.

image

@merwok
Copy link
Member

merwok commented Jun 23, 2023

Just a note that I’m meditating on https://hynek.me/articles/python-subclassing-redux/ and linked articles at the moment and wondering if controlling subclass explosion in pathlib should be a guideline 🙂

@barneygale
Copy link
Contributor Author

barneygale commented Jun 23, 2023

Oh I totally agree. I'd prefer to add only pathlib.AbstractPath, but I'm prevented by the fact that PurePath has an __fspath__() method :)

And if I were to do pathlib over again, I'd scrap the *WindowsPath and *PosixPath classes. That could be controlled by an argument to the PurePath initialiser instead. Oh well...

@barneygale barneygale marked this pull request as draft June 23, 2023 11:57
@barneygale
Copy link
Contributor Author

I've converted this back to a draft as I'm still in two minds about how to implement this. It's possible I'll add _AbstractPath into this PR, or close this PR and log another, or something else. Sorry for the noise

@barneygale
Copy link
Contributor Author

Friendship ended with #104810, now #106043 is my best friend.

@barneygale barneygale closed this Jun 23, 2023
@CAM-Gerlach
Copy link
Member

@barneygale

image

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.

6 participants