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

Jump to bottom

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 10 commits into from

Conversation

uriyyo
Copy link
Member

@uriyyo uriyyo commented Apr 21, 2020 •
edited by bedevere-bot

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

#74033

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

@remilapeyre remilapeyre left a comment

Thanks for writing the fix for this issue Yurii!

Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst Outdated Show resolved Hide resolved
remilapeyre
remilapeyre approved these changes Apr 21, 2020
View changes
Copy link
Contributor

@remilapeyre remilapeyre left a comment

Thanks!

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

@remilapeyre remilapeyre left a comment

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 changes
Copy link
Contributor

@remilapeyre remilapeyre left a comment •
edited

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

I just proposed two small things

Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst Outdated Show resolved Hide resolved
uriyyo and others added 2 commits May 22, 2020
@uriyyo
Update Lib/test/test_pathlib.py
53c4b58 
Co-authored-by: Rémi Lapeyre <remi.lapeyre@henki.fr>
@uriyyo
Update Misc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst
7bbc2a3 
Co-authored-by: Rémi Lapeyre <remi.lapeyre@henki.fr>
@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
Open
@iritkatriel iritkatriel requested a review from brettcannon as a code owner Dec 6, 2022
@netlify
Copy link

netlify bot commented Dec 6, 2022 •
edited

✅ 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 changes
Copy link
Member

@brettcannon brettcannon left a comment

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.

Misc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
@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.

@bedevere-bot bedevere-bot requested a review from brettcannon Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remilapeyre remilapeyre

@brettcannon brettcannon

Assignees
No one assigned
Labels
awaiting change review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@uriyyo @bedevere-bot @brettcannon @remilapeyre @the-knights-who-say-ni @ezio-melotti @iritkatriel