GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-24132: Add direct subclassing of PurePath/Path in pathlib by kfollstad · Pull Request #26906 · python/cpython

Conversation

@kfollstad

@kfollstad kfollstad commented Jun 24, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I submit for your consideration a new working version (with documentation) of an extensible, subclassable PurePath/Path to close bpo-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 any non 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 it becomes 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

barneygale reacted with thumbs up emoji barneygale and Saransh-cpp reacted with hooray emoji
All reactions
  • 👍 1 reaction
  • 🎉 2 reactions
kfollstad added 7 commits June 24, 2021 13:54
@kfollstad
bpo-24132: Add tests for class properties in pathlib's PurePath/Path
710f577
Add tests to verify previously untested black-box class properties of
PurePath and Path in pathlib. Collectively, these tests verify all the
ways the classes represent themselves via isinstance, issubclass,
__repr__(), etc. Moreover all class instances spawned by PurePath or
Path are also similarly tested.
@kfollstad
bpo-24132: Add test for direct subclassing of PurePath/Path in pathlib
a373969
Add expected failing tests to pathlib that will verify the ability
of PurePath and Path to be subclassed by users. As part of this,
test all of the black-box class properties of the newly subclassed
Path-like class instances, and the instances generated by their methods.
@kfollstad
bpo-24132: Fix inconsistent organization of is_mount in pathlib
35d8f78
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.
@kfollstad
bpo-24132: Replace additional hardcoded references in pathlib
bab67b4
Replace hardcorded references to pathlib.Path to facilitate making
Path extensible.
@kfollstad
bpo-24132: Replace factory functionality with masquerade in pathlib
67faae0
Replace factory functionality of PurePath and Path with a psuedo-factory
which appears to generate it's derived subclasses, but instead just
masquerades the new instance as one of the subclasses. Doing so makes
the resultant versions of PurePath and Path directly subclassable and as
such allows them to pass all of the previously failing subclass tests.
@kfollstad
bpo-24132: Add tests for comparing subclasses of PurePath in pathlib
610f41e
Add tests mirroring existing comparison tests for PurePosixPath
and PureWindowsPath but extending them to direct subclasses of PurePath.
@kfollstad
bpo-24132: Document direct subclassing of PurePath/Path in pathlib
a0c4998
Add documentation highlighting the new direct subclass functionality of
PurePath and Path. Also add new is_posix & is_windows doctest conditions
so that system specific doctest code can still be tested instead of just
being taken as a literal block and always skipped.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jun 24, 2021
@bedevere-bot bedevere-bot added the awaiting review label Jun 24, 2021
@barneygale

barneygale commented Jun 25, 2021

Copy link
Copy Markdown
Contributor

Great stuff! Will review in the next couple of days.

kfollstad reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kfollstad
kfollstad commented Jun 25, 2021
View reviewed changes
Comment thread Doc/conf.py
from os import name as system_name
is_posix = system_name == 'posix'
is_windows = system_name == 'nt'
del system_name

@kfollstad kfollstad Jun 25, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@merwok merwok Jan 5, 2022

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@kfollstad kfollstad mentioned this pull request Jun 25, 2021
bpo-24132: Direct sub-classing of pathlib.Path #26438
Closed
@kfollstad
kfollstad marked this pull request as draft June 26, 2021 08:44
@github-actions

github-actions Bot commented Jul 27, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Jul 27, 2021
merwok
merwok reviewed Jan 5, 2022
View reviewed changes
Comment thread Doc/library/pathlib.rst
class will not generate a differently named class:

.. doctest::
:pyversion: > 3.11

@merwok merwok Jan 5, 2022

Copy link
Copy Markdown
Member

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 >=?)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
merwok
merwok reviewed Jan 5, 2022
View reviewed changes
Comment thread Doc/library/pathlib.rst
>>> [Path('/dir').is_absolute, MyPath('/dir').is_absolute()]
[True, True]
>>> [Path().home().drive, MyPath().home().drive]
['', '']

@merwok merwok Jan 5, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@brettcannon brettcannon added the topic-pathlib label Jun 21, 2022
@projetmbc projetmbc mannequin mentioned this pull request Jun 21, 2022
Direct sub-classing of pathlib.Path #68320
Closed
@barneygale

barneygale commented Jun 25, 2022

Copy link
Copy Markdown
Contributor

Hey @kfollstad, FYI, I've now got another PR up that makes subclassing possible: #31691

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Aug 10, 2022
@barneygale

barneygale commented Feb 18, 2023

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@barneygale barneygale closed this Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok left review comments
@barneygale barneygale Awaiting requested review from barneygale barneygale will be requested when the pull request is marked ready for review barneygale is a code owner
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner will be requested when the pull request is marked ready for review AA-Turner is a code owner
@hugovk hugovk Awaiting requested review from hugovk hugovk will be requested when the pull request is marked ready for review hugovk is a code owner
@StanFromIreland StanFromIreland Awaiting requested review from StanFromIreland StanFromIreland will be requested when the pull request is marked ready for review StanFromIreland is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review topic-pathlib

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@kfollstad @barneygale @merwok @brettcannon @the-knights-who-say-ni @ezio-melotti @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.