Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed

Conversation

@barneygale
Copy link
Contributor

@barneygalebarneygale commentedDec 23, 2022
edited
Loading

Add optionalblueprint argument topathlib.PurePath andPath. This argument is supplied whenever a derivative path is created, such as fromPurePath.parent. Subclasses may use this to pass information between paths.

@barneygalebarneygale changed the titleAddpathlib.PurePath.makepath(); unify path object constructiongh-100479: Addpathlib.PurePath.makepath(); unify path object constructionDec 23, 2022
@barneygale
Copy link
ContributorAuthor

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

@barneygalebarneygale changed the titlegh-100479: Addpathlib.PurePath.makepath(); unify path object constructiongh-100479: Addpathlib.PurePath.makepath()Apr 3, 2023
@barneygalebarneygale marked this pull request as ready for reviewApril 3, 2023 21:05
@barneygale
Copy link
ContributorAuthor

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

@zmievsazmievsa 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.

barneygale reacted with thumbs up emoji
@barneygale
Copy link
ContributorAuthor

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. removingPath._accessor. It's hard to make progress without doing this.

AlexWaygood and zmievsa reacted with thumbs up emoji

@AlexWaygoodAlexWaygood self-requested a reviewApril 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 months2. 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 reacted with heart emoji

@barneygale
Copy link
ContributorAuthor

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

AlexWaygood reacted with thumbs up emoji

@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

barneygale reacted with heart emoji

Copy link
Member

@AlexWaygoodAlexWaygood 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
ContributorAuthor

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

AlexWaygood reacted with heart emoji

@barneygalebarneygale changed the titlegh-100479: Addpathlib.PurePath.makepath()gh-100479: Add optionaltemplate argument topathlib.PurePathApr 25, 2023
Copy link
Member

@AlexWaygoodAlexWaygood 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 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!

barneygale reacted with thumbs up emoji
@barneygale
Copy link
ContributorAuthor

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

AlexWaygood reacted with heart emoji

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

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

barneygale reacted with thumbs up emoji
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?

barneygale reacted with thumbs up emoji
Copy link
ContributorAuthor

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.

Copy link
ContributorAuthor

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?

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 thattemplate: Self | None here? I.e. that if template is not None, it will be an instance of the current (user-defined) class.

A

@barneygalebarneygale changed the titlegh-100479: Add optionaltemplate argument topathlib.PurePathgh-100479: Add optionalblueprint argument topathlib.PurePathApr 27, 2023
@barneygale
Copy link
ContributorAuthor

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

AlexWaygood and Gobot1234 reacted with thumbs up emoji

@barneygalebarneygale marked this pull request as draftApril 28, 2023 18:11
@barneygalebarneygale changed the titlegh-100479: Add optionalblueprint argument topathlib.PurePathGH-100479: Add optionalblueprint argument topathlib.PurePathApr 29, 2023
@barneygale
Copy link
ContributorAuthor

Closing in favour of#103975.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@merwokmerwokmerwok left review comments

@AA-TurnerAA-TurnerAA-Turner left review comments

@AlexWaygoodAlexWaygoodAlexWaygood approved these changes

+2 more reviewers

@zmievsazmievsazmievsa approved these changes

@Gobot1234Gobot1234Gobot1234 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@barneygale@merwok@AlexWaygood@Gobot1234@AA-Turner@zmievsa@hauntsaninja@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp