Skip to content

gh-74033: Fix bug when Path takes and ignores **kwargs #19632

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.

Sign up for GitHub

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

Jump to bottom
Merged
miss-islington merged 13 commits into from
Jan 14, 2023
Merged

gh-74033: Fix bug when Path takes and ignores **kwargs #19632

miss-islington merged 13 commits into from
Jan 14, 2023

Conversation

@uriyyo
Copy link
Member

@uriyyo uriyyo commented Apr 21, 2020 •
edited by AlexWaygood
Loading

Fix a bug where Path takes and ignores **kwargs by adding to PurePath class __init__ method which can take only positional arguments.

Automerge-Triggered-By: GH:brettcannon

remilapeyre
remilapeyre reviewed Apr 21, 2020
View reviewed changes
Copy link

@remilapeyre remilapeyre left a comment

Choose a reason for hiding this comment

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

Thanks for writing the fix for this issue Yurii!

remilapeyre
remilapeyre approved these changes Apr 21, 2020
View reviewed changes
Copy link

@remilapeyre remilapeyre left a comment

Choose a reason for hiding this comment

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

Thanks!

remilapeyre
remilapeyre suggested changes May 21, 2020
View reviewed changes
Copy link

@remilapeyre remilapeyre left a comment

Choose a reason for hiding this comment

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

Hi @uriyyo, as I indicated on bpo, PurePath suffers from a similar issue, it and its subclasses cannot accept **kwargs.

Could you make the same changes to PurePath so the behaviour is the same for the whole module?

@uriyyo
Copy link
Member Author

uriyyo commented May 21, 2020

Hi @remilapeyre
Sure, I will update this PR.

@uriyyo
Copy link
Member Author

uriyyo commented May 21, 2020

Hi @remilapeyre
I update PR, can you review it?

remilapeyre
remilapeyre approved these changes May 21, 2020
View reviewed changes
Copy link

@remilapeyre remilapeyre left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @uriyyo, I just tested your changes and it looks good to me 👍

I just proposed two small things

@uriyyo
Copy link
Member Author

uriyyo commented May 22, 2020

@remilapeyre I agree with your suggestions, thanks.

@uriyyo uriyyo changed the title bpo-29847: Fix bug when Path takes and ignores **kwargs bpo-29847: Fix bug when PurePath takes and ignores **kwargs May 22, 2020
@uriyyo uriyyo changed the title bpo-29847: Fix bug when PurePath takes and ignores **kwargs bpo-29847: Fix bug when Path takes and ignores **kwargs May 22, 2020
@JelleZijlstra JelleZijlstra mentioned this pull request Oct 10, 2022
Closed
@netlify
Copy link

netlify bot commented Dec 6, 2022 •
edited
Loading

✅ Deploy Preview for python-cpython-preview canceled.

Name Link
🔨 Latest commit 8ef2e9e
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/638fc5db834885000837776c

@brettcannon brettcannon changed the title bpo-29847: Fix bug when Path takes and ignores **kwargs gh-74033: Fix bug when Path takes and ignores **kwargs Dec 16, 2022
brettcannon
brettcannon requested changes Dec 16, 2022
View reviewed changes
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Since "explicit is better than implicit", I think we want pathlib.Path to raise an error with keyword arguments instead of letting PurePath silently accept keyword arguments.

@bedevere-bot
Copy link

bedevere-bot commented Dec 16, 2022

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@uriyyo
Copy link
Member Author

uriyyo commented Dec 17, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

bedevere-bot commented Dec 17, 2022

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

brettcannon
brettcannon requested changes Dec 23, 2022
View reviewed changes
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Unfortunately we need to ask that this raises a warning for at least 2 releases.

@bedevere-bot
Copy link

bedevere-bot commented Dec 23, 2022

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@uriyyo
Copy link
Member Author

uriyyo commented Dec 24, 2022

I have made the requested changes; please review again.

@bedevere-bot
Copy link

bedevere-bot commented Dec 24, 2022

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

brettcannon
brettcannon approved these changes Jan 13, 2023
View reviewed changes
@miss-islington
Copy link
Contributor

miss-islington commented Jan 14, 2023

Status check is done, and it's a success ✅.

@miss-islington miss-islington merged commit 080cb27 into python:main Jan 14, 2023
@brettcannon
Copy link
Member

brettcannon commented Jan 14, 2023

Thanks for the PR and the patience!

@uriyyo uriyyo deleted the fix-issue-29847 branch January 14, 2023 08:24
@takluyver takluyver mentioned this pull request Sep 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon brettcannon approved these changes

@barneygale barneygale Awaiting requested review from barneygale

+1 more reviewer

@remilapeyre remilapeyre remilapeyre approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

topic-pathlib

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@uriyyo @bedevere-bot @miss-islington @brettcannon @remilapeyre @hauntsaninja @the-knights-who-say-ni @ezio-melotti @iritkatriel