GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-74033: Fix bug when Path takes and ignores **kwargs by uriyyo · Pull Request #19632 · python/cpython

Conversation

@uriyyo

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

Uh oh!

There was an error while loading. Please reload this page.

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.

  • Issue: Path takes and ignores **kwargs #74033

Automerge-Triggered-By: GH:brettcannon

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@uriyyo
bpo-29847: Fix bug when Path takes and ignores **kwargs
4228d60
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 21, 2020
@bedevere-bot bedevere-bot added the awaiting review label Apr 21, 2020
remilapeyre
remilapeyre reviewed Apr 21, 2020
View reviewed changes

@remilapeyre remilapeyre left a comment

Copy link
Copy Markdown
Contributor

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.

Thanks for writing the fix for this issue Yurii!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
uriyyo and others added 2 commits April 21, 2020 13:20
@uriyyo Rémi Lapeyre
Update Misc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst
72ffd24 
Co-Authored-By: Rémi Lapeyre <[email protected]>
@uriyyo
Move tests from PathTest to _BasePathTest class
fc740ec
remilapeyre
remilapeyre approved these changes Apr 21, 2020
View reviewed changes

@remilapeyre remilapeyre left a comment

Copy link
Copy Markdown
Contributor

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.

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Apr 21, 2020
remilapeyre
remilapeyre suggested changes May 21, 2020
View reviewed changes

@remilapeyre remilapeyre left a comment

Copy link
Copy Markdown
Contributor

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@uriyyo

uriyyo commented May 21, 2020

Copy link
Copy Markdown
Member Author

Hi @remilapeyre
Sure, I will update this PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@uriyyo
Fix bug when PurePath takes and ignores **kwargs
7dab30d
@uriyyo

uriyyo commented May 21, 2020

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
remilapeyre
remilapeyre approved these changes May 21, 2020
View reviewed changes

@remilapeyre remilapeyre left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

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

I just proposed two small things

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
uriyyo and others added 2 commits May 22, 2020 09:56
@uriyyo Rémi Lapeyre
Update Lib/test/test_pathlib.py
53c4b58 
Co-authored-by: Rémi Lapeyre <[email protected]>
@uriyyo Rémi Lapeyre
Update Misc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst
7bbc2a3 
Co-authored-by: Rémi Lapeyre <[email protected]>
@uriyyo

uriyyo commented May 22, 2020

Copy link
Copy Markdown
Member Author

@remilapeyre I agree with your suggestions, thanks.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@JelleZijlstra JelleZijlstra mentioned this pull request Oct 10, 2022
Path takes and ignores **kwargs #74033
Closed
@iritkatriel
Merge branch 'main' into fix-issue-29847
8ef2e9e
@iritkatriel
iritkatriel requested a review from brettcannon as a code owner December 6, 2022 22:44
@netlify

netlify Bot commented Dec 6, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

✅ 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

@brettcannon brettcannon left a comment

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting core review label Dec 16, 2022
@bedevere-bot

bedevere-bot commented Dec 16, 2022

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Dec 16, 2022
@uriyyo @brettcannon
Update Misc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst
3311d75 
Co-authored-by: Brett Cannon <[email protected]>
@uriyyo
Remove kwargs from pathlib.Path; Remove redundant subclass test
f021d2b
@uriyyo

uriyyo commented Dec 17, 2022

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Dec 17, 2022
@bedevere-bot

bedevere-bot commented Dec 17, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from brettcannon December 17, 2022 08:48
brettcannon
brettcannon requested changes Dec 23, 2022
View reviewed changes

@brettcannon brettcannon left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting change review label Dec 23, 2022
@bedevere-bot

bedevere-bot commented Dec 23, 2022

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Dec 23, 2022
@uriyyo
Add deprecation warning when calling Path with **kwargs
6fa22de
@uriyyo

uriyyo commented Dec 24, 2022

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Dec 24, 2022
@bedevere-bot

bedevere-bot commented Dec 24, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from brettcannon December 24, 2022 08:53
@hauntsaninja hauntsaninja added the topic-pathlib label Jan 7, 2023
brettcannon
brettcannon approved these changes Jan 13, 2023
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Jan 13, 2023
@brettcannon
brettcannon requested a review from barneygale as a code owner January 13, 2023 23:40
brettcannon added 2 commits January 13, 2023 15:40
@brettcannon
Update Misc/NEWS.d/next/Library/2020-04-18-17-45-03.bpo-29847.Uxtbq0.rst
c654dda
@brettcannon
Merge branch 'main' into fix-issue-29847
71f662f
@brettcannon brettcannon added the 🤖 automerge label Jan 13, 2023
@miss-islington

miss-islington commented Jan 14, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington
miss-islington merged commit 080cb27 into python:main Jan 14, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Jan 14, 2023
@brettcannon

brettcannon commented Jan 14, 2023

Copy link
Copy Markdown
Member

Thanks for the PR and the patience!
uriyyo reacted with hooray emoji uriyyo reacted with heart emoji
All reactions
  • 🎉 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@uriyyo
uriyyo deleted the fix-issue-29847 branch January 14, 2023 08:24
@takluyver takluyver mentioned this pull request Sep 27, 2023
Remove destination data files if they exist. pypa/flit#658
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic-pathlib

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@uriyyo @bedevere-bot @miss-islington @brettcannon @remilapeyre @hauntsaninja @the-knights-who-say-ni @ezio-melotti @iritkatriel
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.