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-100479: Add optional blueprint argument to pathlib.PurePath #100481

Closed
wants to merge 22 commits into from

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented Dec 23, 2022

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.

@barneygale 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
Lib/pathlib.py Outdated Show resolved Hide resolved
@barneygale
Copy link
Contributor Author

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!

@merwok

This comment was marked as resolved.

@barneygale

This comment was marked as resolved.

@barneygale barneygale changed the title gh-100479: Add pathlib.PurePath.makepath(); unify path object construction gh-100479: Add pathlib.PurePath.makepath() Apr 3, 2023
@barneygale barneygale marked this pull request as ready for review April 3, 2023 21:05
@barneygale
Copy link
Contributor Author

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.

Copy link
Contributor

@zmievsa zmievsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
@barneygale
Copy link
Contributor Author

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.

@AlexWaygood AlexWaygood self-requested a review April 5, 2023 22:43
@AlexWaygood
Copy link
Member

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

@barneygale
Copy link
Contributor Author

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

@Gobot1234
Copy link
Contributor

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

Lib/pathlib.py Outdated Show resolved Hide resolved
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@barneygale
Copy link
Contributor Author

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

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

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Doc/library/pathlib.rst Outdated Show resolved Hide resolved
@barneygale
Copy link
Contributor Author

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

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

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -159,7 +159,7 @@ we also call *flavours*:

class MyPath(PurePosixPath):
def __init__(self, *pathsegments, template=None, session_id=None):
super().__init__(*pathsegments)
super().__init__(*pathsegments, template=template)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Doc/library/pathlib.rst Outdated Show resolved Hide resolved
Comment on lines 153 to 156
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::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@barneygale 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
Copy link
Contributor Author

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

@barneygale barneygale marked this pull request as draft April 28, 2023 18:11
@barneygale 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
@barneygale
Copy link
Contributor Author

Closing in favour of #103975.

@barneygale barneygale closed this May 4, 2023
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.

None yet

8 participants