GH-100479: Add optional `blueprint` argument to `pathlib.PurePath` by barneygale · Pull Request #100481 · python/cpython · GitHub

Add optional blueprint argument to pathlib.PurePath and Path. This argument is supplied whenever a derivative path is created, such as from PurePath.parent. Subclasses may use this to pass information between paths.

Issue: Support for sharing state between pathlib subclasses #100479

Hey @serhiy-storchaka, I believe you're pretty familiar with the pathlib internals. Penny for your initial impressions of this patch? It does cut out a key part of the initial pathlib design: the idea of skipping re-parsing and re-normalizing paths in case where we can be sure it's not necessary (e.g. in path.parents, path.iterdir()). I'd love to be able to convince you this is a good idea!

This is now ready for review! My previous comment no longer applies, as we fixed the pathlib construction weirdness in #102789. This change should be pretty performance-neutral now.

After looking through the changes, this is an obvious approve from my side: the changes make a lot sense and we have been waiting for them for a long time.

My biggest concern is that we're adding a breaking change to _from_parsed_parts which is not a big problem on its own because the interface is private, but I would still take a look around for some relatively popular libraries that might actually be dependent on that interface.

My biggest concern is that we're adding a breaking change to _from_parsed_parts which is not a big problem on its own because the interface is private, but I would still take a look around for some relatively popular libraries that might actually be dependent on that interface.

Well, it's undocumented and underscore-prefixed. We've been breaking other things like that over the last few months, e.g. removing Path._accessor. It's hard to make progress without doing this.

I'm also slightly concerned that this design might be a little fragile. The following situation seems fairly plausible:
1. Barney takes a break from open source for a few months

2. During Barney's break, a contributor files a PR to add `awesome_new_method` to pathlib. The method is so awesome, it's immediately clear to the core dev team that we desperately need the method.

3. Alex merges the PR, but because Barney's on an open-source break, there's nobody to remind us that the new method should really be calling `self.makepath(*args)` instead of `self.__class__(*args)`. Support for sharing state between pathlib subclasses is now broken :(
Is there a way we can guard against this?
Hm. I expect a user will spot that it doesn't work and log a bug. It doesn't have any wider consequences, I don't think.

Okay, I chatted with Brandt at the sprint, and he persuaded me that I was worrying too much about this hypothetical scenario :)

FWIW, I am open to alternatives to this new method. There are things we can do like pass an opaque object around, or derive a new type to 'bind' the context. I went back and forth with these, and in the end went with this approach as it appears to be the simplest. But happy to look again if you have suggestions :)

Also as a bit of a testimonial I really like the design and have the changes for something I locally implemented for this PR and it makes my code significantly nicer

The implementation and the docs here look great. Thanks for addressing my concerns, and for your patience :)

My only remaining reservation is about the name of the method itself. I think I would still prefer "newpath" -- but if you'd like more opinions, you could always start a thread on Discourse or Discord :)

Thanks so much for the thorough review! I'll open a thread about the method name shortly.

Wow, using the template argument instead of adding the makepath method makes it so much easier for me to understand what's going on! This is a great improvement!

Yeah, I was missing the wood for the trees there. Massive improvement.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

LGTM. I guess the only remaining question is whether "template" is the best name for this new parameter. I'm fine with "template", but you might want to head back to Discourse if you want some more bikeshedding :)

Should we add a test to make sure diamond inheritance works?

I'm not sure. There's only one place in pathlib.py where we call super(), and that only exists because we need to raise a deprecation warning when additional arguments are supplied to pathlib.Path(). The Path.__init__() method will be removed in 3.14, at which point it will be impossible for the test to fail.

Perhaps a hidden .. doctest:: block would be best?

Does it make sense to specify that template: Self | None here? I.e. that if template is not None, it will be an instance of the current (user-defined) class.

A

I've created a branch/PR that uses a __newpath__() method, for comparison's sake:

GH-100479: Add pathlib.PurePath.with_segments() #103975

Closing in favour of #103975.

Add pathlib.PurePath.makepath(); unify path object construction

a6fdd0e

bedevere-bot added the awaiting review label Dec 23, 2022

barneygale changed the title Add pathlib.PurePath.makepath(); unify path object construction gh-100479: Add pathlib.PurePath.makepath(); unify path object construction Dec 23, 2022

bedevere-bot mentioned this pull request Dec 23, 2022

Support for sharing state between pathlib subclasses #100479

Closed

merwok reviewed Dec 24, 2022

View reviewed changes

Comment thread Lib/pathlib.py

Fix reST role name.

b061747

Move call to os.getcwd() back into Path.cwd()

99eb8b1

barneygale force-pushed the gh-100479-add-makepath branch from 19bf6ff to 99eb8b1 Compare December 24, 2022 17:57

This comment was marked as resolved.

Sign in to view

This was referenced Jan 5, 2023

Path takes and ignores **kwargs #74033

Closed

pathlib.Path._from_parsed_parts should call cls.__new__(cls) #76846

Closed

subclasses of pathlib.PurePosixPath never call __init__ or __new__ #85281

Closed

Merge branch 'main' into pythongh-100479 -add-makepath

4759d01

hauntsaninja added the topic-pathlib label Jan 7, 2023

Merge branch 'main' into pythongh-100479 -add-makepath

ef6f4c3

barneygale changed the title gh-100479: Add pathlib.PurePath.makepath(); unify path object construction gh-100479: Add pathlib.PurePath.makepath() Apr 3, 2023

Add news blurb.

595b8ae

barneygale marked this pull request as ready for review April 3, 2023 21:05

barneygale mentioned this pull request Apr 3, 2023

bpo-24132: Add pathlib._AbstractPath #31085

Closed

zmievsa approved these changes Apr 4, 2023

View reviewed changes

Comment thread Lib/pathlib.py Outdated

Comment thread Lib/pathlib.py Outdated

bedevere-bot added awaiting core review and removed awaiting review labels Apr 4, 2023

AlexWaygood self-requested a review April 5, 2023 22:43

barneygale added 3 commits April 9, 2023 18:45

Merge branch 'main' into pythongh-100479 -add-makepath

dcfe70a

Add whatsnew entry

117fe4b

Merge branch 'main' into pythongh-100479 -add-makepath

e75dedc

AlexWaygood reviewed Apr 24, 2023

View reviewed changes

Comment thread Lib/pathlib.py

barneygale added 2 commits April 25, 2023 15:24

Restore _PathParents

ae48454

Add note to parents about potential reference cycle.

e7a8fe3

AlexWaygood approved these changes Apr 25, 2023

View reviewed changes

Replace makepath() method with template initialiser argument.

7f12faa

barneygale changed the title gh-100479: Add pathlib.PurePath.makepath() gh-100479: Add optional template argument to pathlib.PurePath Apr 25, 2023

barneygale requested a review from AlexWaygood April 25, 2023 17:17

AlexWaygood reviewed Apr 25, 2023

View reviewed changes

Comment thread Doc/library/pathlib.rst Outdated

Comment thread Misc/NEWS.d/next/Library/2023-04-03-22-02-35.gh-issue-100479.kNBjQm.rst Outdated

Apply suggestions from code review

687c764

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood approved these changes Apr 25, 2023

View reviewed changes

barneygale added 2 commits April 25, 2023 18:46

Fix docs for other classes.

d7e326a

Pass template to super() to support diamond inheritance.

a65d499

AlexWaygood reviewed Apr 26, 2023

View reviewed changes

Fixed missed template argument to super().

d4b15d7

barneygale commented Apr 26, 2023

View reviewed changes

Comment thread Doc/library/pathlib.rst Outdated

AA-Turner reviewed Apr 27, 2023

View reviewed changes

template --> blueprint

958b183

barneygale changed the title gh-100479: Add optional template argument to pathlib.PurePath gh-100479: Add optional blueprint argument to pathlib.PurePath Apr 27, 2023

barneygale mentioned this pull request Apr 28, 2023

GH-100479: Add pathlib.PurePath.with_segments() #103975

Merged

barneygale marked this pull request as draft April 28, 2023 18:11

barneygale changed the title gh-100479: Add optional blueprint argument to pathlib.PurePath GH-100479: Add optional blueprint argument to pathlib.PurePath Apr 29, 2023

Merge branch 'main' into pythongh-100479 -add-makepath

1e10188

barneygale closed this May 4, 2023

                     class MyPath(PurePosixPath):
                         def __init__(self, *pathsegments, template=None, session_id=None):
-                            super().__init__(*pathsegments)
+                            super().__init__(*pathsegments, template=template)

+                 The optional *template* argument may provide another path object. It is
+                 supplied whenever a new path object is created from an existing one, such
+                 as in :attr:`parent` or :meth:`relative_to`. Subclasses may use this to
+                 pass information between path objects. For example::

GH-100479: Add optional `blueprint` argument to `pathlib.PurePath` by barneygale · Pull Request #100481 · python/cpython · GitHub

Uh oh!

Uh oh!

GH-100479: Add optional blueprint argument to pathlib.PurePath - #100481

GH-100479: Add optional blueprint argument to pathlib.PurePath#100481barneygale wants to merge 22 commits intopython:mainpython/cpython:mainfrom barneygale:gh-100479-add-makepathbarneygale/cpython:gh-100479-add-makepathCopy head branch name to clipboard

Conversation

barneygale commented Dec 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

barneygale commented Dec 24, 2022

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

barneygale commented Apr 3, 2023

Uh oh!

zmievsa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

barneygale commented Apr 5, 2023

Uh oh!

AlexWaygood commented Apr 24, 2023

Uh oh!

barneygale commented Apr 24, 2023

Uh oh!

Gobot1234 commented Apr 24, 2023

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

barneygale commented Apr 25, 2023

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

barneygale commented Apr 25, 2023

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

barneygale Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

barneygale Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AA-Turner Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

barneygale commented Apr 28, 2023

Uh oh!

barneygale commented May 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

GH-100479: Add optional blueprint argument to pathlib.PurePath#100481
barneygale wants to merge 22 commits into
python:mainfrom
barneygale:gh-100479-add-makepath

barneygale commented Dec 23, 2022 •
edited
Loading