Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
GH-100479: Add optionalblueprint argument topathlib.PurePath#100481
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
pathlib.PurePath.makepath(); unify path object constructionpathlib.PurePath.makepath(); unify path object constructionUh oh!
There was an error while loading.Please reload this page.
barneygale commentedDec 24, 2022
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 |
19bf6ff to99eb8b1Compare This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
pathlib.PurePath.makepath(); unify path object constructionpathlib.PurePath.makepath()barneygale commentedApr 3, 2023
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. |
zmievsa left a comment
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.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
barneygale commentedApr 5, 2023
Well, it's undocumented and underscore-prefixed. We've been breaking other things like that over the last few months, e.g. removing |
AlexWaygood commentedApr 24, 2023
Okay, I chatted with Brandt at the sprint, and he persuaded me that I was worrying too much about this hypothetical scenario :) |
barneygale commentedApr 24, 2023
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 commentedApr 24, 2023
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 |
Uh oh!
There was an error while loading.Please reload this page.
AlexWaygood left a comment
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.
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 commentedApr 25, 2023
Thanks so much for the thorough review! I'll open a thread about the method name shortly. |
pathlib.PurePath.makepath()template argument topathlib.PurePath
AlexWaygood left a comment
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.
Wow, using thetemplate argument instead of adding themakepath method makes it so much easier for me to understand what's going on! This is a great improvement!
Uh oh!
There was an error while loading.Please reload this page.
Misc/NEWS.d/next/Library/2023-04-03-22-02-35.gh-issue-100479.kNBjQm.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
barneygale commentedApr 25, 2023
Yeah, I was missing the wood for the trees there. Massive improvement. |
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
AlexWaygood left a comment
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.
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 :)
Doc/library/pathlib.rst Outdated
| class MyPath(PurePosixPath): | ||
| def __init__(self, *pathsegments, template=None, session_id=None): | ||
| super().__init__(*pathsegments) | ||
| super().__init__(*pathsegments, template=template) |
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.
Should we add a test to make sure diamond inheritance works?
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.
I'm not sure. There's only one place inpathlib.py where we callsuper(), and that only exists because we need to raise a deprecation warning when additional arguments are supplied topathlib.Path(). ThePath.__init__() method will be removed in 3.14, at which point it will be impossible for the test to fail.
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.
Perhaps a hidden.. doctest:: block would be best?
Uh oh!
There was an error while loading.Please reload this page.
Doc/library/pathlib.rst Outdated
| 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:: |
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.
Does it make sense to specify thattemplate: Self | None here? I.e. that if template is not None, it will be an instance of the current (user-defined) class.
A
template argument topathlib.PurePathblueprint argument topathlib.PurePathbarneygale commentedApr 28, 2023
I've created a branch/PR that uses a |
blueprint argument topathlib.PurePathblueprint argument topathlib.PurePathbarneygale commentedMay 4, 2023
Closing in favour of#103975. |
Uh oh!
There was an error while loading.Please reload this page.
Add optionalblueprint argument to
pathlib.PurePathandPath. This argument is supplied whenever a derivative path is created, such as fromPurePath.parent. Subclasses may use this to pass information between paths.