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 — bpo-18108: Adding dir_fd and follow_symlinks keyword args to shutil.chown by ta1hia · Pull Request #15811 · python/cpython

Conversation

@ta1hia

@ta1hia ta1hia commented Sep 9, 2019 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

In addition to adding the kwargs, I extended the existing test_shutil.TestShutil.test_chown unit test and also added to the documentation (shutil.chown docstring and Docs/library/shutil.chown function description).

Co-Authored-By: Berker Peksang [email protected]

https://bugs.python.org/issue18108

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Sep 9, 2019
@bedevere-bot bedevere-bot added the awaiting review label Sep 9, 2019
zware
zware previously requested changes Sep 11, 2019
View reviewed changes

@zware zware left a comment •
edited
Loading

Uh oh!

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

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.

A couple of minor issues, but otherwise this LGTM. Adding @berkerpeksag and @vstinner as others who may be better suited to review this (as Berker wrote the original patch on the issue, and Victor added these parameters to the os functions).

Sorry, something went wrong.

Uh oh!

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

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting review label Sep 11, 2019
@bedevere-bot

bedevere-bot commented Sep 11, 2019

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 Sep 11, 2019
@zware
zware requested review from berkerpeksag and vstinner September 11, 2019 11:34
@berkerpeksag

berkerpeksag commented Sep 11, 2019

Copy link
Copy Markdown
Member

I'm not going to review or merge this PR as my patch was taken without my approval on the issue tracker and my name wasn't even mentioned in the new PR.
All reactions

Sorry, something went wrong.

Uh oh!

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

ta1hia commented Sep 12, 2019

Copy link
Copy Markdown
Contributor Author

I'm not going to review or merge this PR as my patch was taken without my approval on the issue tracker and my name wasn't even mentioned in the new PR.

It's true, @berkerpeksag uploaded a patch to the bug tracker for this already and I failed to check with him before submitting this PR. My apologies - I grabbed what seemed like a stale issue without following the proper protocol. I can close this PR if that's what makes the most sense at this point.
All reactions

Sorry, something went wrong.

Uh oh!

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

zware commented Sep 12, 2019

Copy link
Copy Markdown
Member

I personally think it would be OK to add a Co-Authored-By: "header" to include @berkerpeksag's authorship if he's alright with that, but I'll leave that up to him. I do note that there are significant changes here compared to Berker's original patch, though a large chunk of it is still his work.

@berkerpeksag, would you mind to either sign off on a Co-Authored-By credit or close the PR?
All reactions

Sorry, something went wrong.

Uh oh!

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

berkerpeksag commented Sep 12, 2019

Copy link
Copy Markdown
Member

I left my earlier comment because I was asked to review the PR. I'll let Zachary and/or Victor decide what to do next as I have no intention to block merging this PR.
All reactions

Sorry, something went wrong.

Uh oh!

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

ta1hia commented Sep 16, 2019

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

As for adding Berker as a co-author, would it be best to do that in a new commit?
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 Sep 16, 2019
@bedevere-bot

bedevere-bot commented Sep 16, 2019

Copy link
Copy Markdown

Thanks for making the requested changes!

@zware: 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 zware September 16, 2019 21:34
serhiy-storchaka
serhiy-storchaka reviewed Jan 3, 2022
View reviewed changes
Comment thread Doc/library/shutil.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Library/2019-09-09-18-18-34.bpo-18108.ajPLAO.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@ghost

ghost commented Dec 14, 2023 •
edited by ghost
Loading

Uh oh!

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

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
serhiy-storchaka dismissed zware’s stale review December 14, 2023 18:09

Suggestions were applied.

@bedevere-app bedevere-app Bot added awaiting review and removed awaiting change review labels Dec 14, 2023
@serhiy-storchaka

serhiy-storchaka commented Dec 14, 2023

Copy link
Copy Markdown
Member

@zware's suggestions were applied. The code was updated to the current main, I made also some minor changes. @ta1hia, please check that I wrote your name correctly in the What's New file.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes Dec 14, 2023
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Dec 14, 2023
@terryjreedy

terryjreedy commented Dec 16, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

@ta1hia Please click the button in #15811 (comment) to re-sign the CLA with your current GH email. It is needed to merge here.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot mentioned this pull request Apr 21, 2024
shutil.chown should support dir_fd and follow_symlinks keyword arguments #62308
Closed
@nineteendo

nineteendo commented Apr 21, 2024

Copy link
Copy Markdown
Contributor

@serhiy-storchaka, could you close this in favor of #118136? We can't merge this pull request if the CLA isn't signed.
I based mine only on the patch of Berker, so there shouldn't be any problems with licensing.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka @berkerpeksag
@ta1hia @zware
bpo-18108: Adding dir_fd and follow_symlinks kwargs to shutil.chown
e464e7b 
* Adding dir_fd and follow_symlinks keyword args to shutil.chown
* Extending test_shutil.TestShutil.test_chown to include new kwargs
* Updating shutil.chown documentation

Co-authored-by: Berker Peksag <[email protected]>
Co-authored-by: Tahia Khan <[email protected]>
Co-authored-by: Zachary Ware <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
@serhiy-storchaka
serhiy-storchaka force-pushed the bpo-18108 branch from 57af913 to e464e7b Compare April 22, 2024 17:52
@serhiy-storchaka

serhiy-storchaka commented Apr 22, 2024

Copy link
Copy Markdown
Member

Thank you @nineteendo, but it was not only the original patch, but some @ta1hia's additions, and suggestions from the review, and finally my changes. We do not want to lose it. I am sure that the problem with the CLA bot is technical, not legal, so we can work it around.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
serhiy-storchaka enabled auto-merge (squash) April 22, 2024 18:00
@serhiy-storchaka
serhiy-storchaka merged commit 8974a63 into python:main Apr 22, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 22, 2024
@nineteendo

nineteendo commented Apr 22, 2024

Copy link
Copy Markdown
Contributor

Yeah, I was able to include all other changes, but not the tests. So it's better this way. 80 more PR's awaiting merge to go:
https://github.com/python/cpython/pulls?q=is%3Aopen+is%3Apr+label%3A%22awaiting+merge%22+sort%3Acreated-asc+-label%3Astale
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Apr 29, 2024

Copy link
Copy Markdown
Member

Nice enhancement!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
@vstinner vstinner Awaiting requested review from vstinner
@zware zware Awaiting requested review from zware

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

10 participants

@ta1hia @bedevere-bot @berkerpeksag @zware @serhiy-storchaka @terryjreedy @nineteendo @vstinner @the-knights-who-say-ni @ezio-melotti
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 01402503 — 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.