Skip to content

GH-89812: Add pathlib._PurePathExt - #104810

Closed
barneygale wants to merge 20 commits into
python:mainfrom
barneygale:gh-89812-add-lexicalpath
Closed

GH-89812: Add pathlib._PurePathExt#104810
barneygale wants to merge 20 commits into
python:mainfrom
barneygale:gh-89812-add-lexicalpath

Conversation

@barneygale

@barneygalebarneygale commented May 23, 2023

Copy link
Copy Markdown
Contributor

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.
@barneygalebarneygale changed the title GH-89812: Add pathlib._LexicalPathGH-89812: Add pathlib._BasePurePathMay 23, 2023
@barneygale

Copy link
Copy Markdown
ContributorAuthor

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

Comment threadLib/pathlib.py
Comment threadLib/pathlib.py Outdated
Comment threadLib/pathlib.py Outdated
Comment threadLib/pathlib.py Outdated
Comment threadLib/pathlib.py Outdated
Comment threadLib/pathlib.py Outdated
Comment threadLib/pathlib.py
barneygaleand others added 2 commits May 24, 2023 16:04
Co-authored-by: Éric <merwok@netwok.org>
Comment threadLib/pathlib.py Outdated
barneygaleand others added 2 commits May 24, 2023 17:36
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Comment threadLib/os.py
Comment threadLib/test/test_pathlib.py Outdated
@barneygale

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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

@barneygalebarneygale changed the title GH-89812: Add pathlib._BasePurePathGH-89812: Add pathlib._PurePathExtJun 22, 2023
@barneygale

barneygale commented Jun 22, 2023

Copy link
Copy Markdown
ContributorAuthor

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

barneygale commented Jun 22, 2023

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
Member

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

barneygale commented Jun 23, 2023

Copy link
Copy Markdown
ContributorAuthor

Oh I totally agree. I'd prefer to add onlypathlib.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
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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

@CAM-Gerlach

Copy link
Copy Markdown
Member

@barneygale

image

Sign up for freeto 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

@barneygale@merwok@CAM-Gerlach@brettcannon@AlexWaygood@bedevere-bot