Skip to content

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

Merged
miss-islington merged 13 commits into
python:mainfrom
uriyyo:fix-issue-29847
Jan 14, 2023
Merged

gh-74033: Fix bug when Path takes and ignores **kwargs#19632
miss-islington merged 13 commits into
python:mainfrom
uriyyo:fix-issue-29847

Conversation

@uriyyo

@uriyyouriyyo commented Apr 21, 2020

Copy link
Copy Markdown
Member

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

@remilapeyreremilapeyre left a comment

Copy link
Copy Markdown

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!

Comment threadLib/test/test_pathlib.py Outdated
Comment threadMisc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst Outdated

@remilapeyreremilapeyre left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks!

@remilapeyreremilapeyre left a comment

Copy link
Copy Markdown

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
Copy Markdown
MemberAuthor

Hi @remilapeyre
Sure, I will update this PR.

@uriyyo

Copy link
Copy Markdown
MemberAuthor

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

@remilapeyreremilapeyre left a comment

Copy link
Copy Markdown

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

Comment threadLib/test/test_pathlib.py Outdated
Comment threadMisc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst Outdated
uriyyoand others added 2 commits May 22, 2020 09:56
Co-authored-by: Rémi Lapeyre <remi.lapeyre@henki.fr>
Co-authored-by: Rémi Lapeyre <remi.lapeyre@henki.fr>
@uriyyo

Copy link
Copy Markdown
MemberAuthor

@remilapeyre I agree with your suggestions, thanks.

@uriyyouriyyo changed the title bpo-29847: Fix bug when Path takes and ignores **kwargsbpo-29847: Fix bug when PurePath takes and ignores **kwargsMay 22, 2020
@uriyyouriyyo changed the title bpo-29847: Fix bug when PurePath takes and ignores **kwargsbpo-29847: Fix bug when Path takes and ignores **kwargsMay 22, 2020
@netlify

netlifyBot commented Dec 6, 2022

Copy link
Copy Markdown

Deploy Preview for python-cpython-preview canceled.

NameLink
🔨 Latest commit8ef2e9e
🔍 Latest deploy loghttps://app.netlify.com/sites/python-cpython-preview/deploys/638fc5db834885000837776c

@brettcannonbrettcannon changed the title bpo-29847: Fix bug when Path takes and ignores **kwargsgh-74033: Fix bug when Path takes and ignores **kwargsDec 16, 2022

@brettcannonbrettcannon left a comment

Copy link
Copy Markdown
Member

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.

Comment threadMisc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst Outdated
Comment threadLib/pathlib.py Outdated
Comment threadLib/test/test_pathlib.py Outdated
@bedevere-bot

Copy link
Copy Markdown

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
Copy Markdown
MemberAuthor

I have made the requested changes; please review again

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@brettcannonbrettcannon left a comment

Copy link
Copy Markdown
Member

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.

Comment threadLib/pathlib.py Outdated
@bedevere-bot

Copy link
Copy Markdown

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
Copy Markdown
MemberAuthor

I have made the requested changes; please review again.

@bedevere-bot

Copy link
Copy Markdown

Thanks for making the requested changes!

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

Comment threadMisc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst Outdated
@miss-islington

Copy link
Copy Markdown
Contributor

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

@miss-islington
miss-islington merged commit 080cb27 into python:mainJan 14, 2023
@brettcannon

Copy link
Copy Markdown
Member

Thanks for the PR and the patience!

@uriyyo
uriyyo deleted the fix-issue-29847 branch January 14, 2023 08:24
Sign up for freeto 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.

9 participants

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