8000 bpo-18108: Adding dir_fd and follow_symlinks keyword args to shutil.chown by ta1hia · Pull Request #15811 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-18108: Adding dir_fd and follow_symlinks keyword args to shutil.chown #15811

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
merged 1 commit into from
Apr 22, 2024
Merged

bpo-18108: Adding dir_fd and follow_symlinks keyword args to shutil.chown #15811

merged 1 commit into from
Apr 22, 2024

Conversation

ta1hia
Copy link
Contributor
@ta1hia ta1hia commented Sep 9, 2019 •
edited
Loading

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 berker.peksag@gmail.com

https://bugs.python.org/issue18108

zware
zware previously requested changes Sep 11, 2019
View reviewed changes
Copy link
Member
@zware zware 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.

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).

@bedevere-bot
Copy link

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.
< 8000 !-- no margin wins, so we check it last and use its value if true. -->

@berkerpeksag
Copy link
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.

@ta1hia
Copy link
Contributor Author
ta1hia commented Sep 12, 2019

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.

@zware
Copy link
Member
zware commented Sep 12, 2019

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?

@berkerpeksag
Copy link
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.

@ta1hia
Copy link
Contributor Author
ta1hia commented Sep 16, 2019

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?

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@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
@ghost
Copy link
ghost commented Dec 14, 2023 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@serhiy-storchaka serhiy-storchaka dismissed zware’s stale review December 14, 2023 18:09

Suggestions were applied.

@serhiy-storchaka
Copy link
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.

@terryjreedy
Copy link
Member
terryjreedy commented Dec 16, 2023 •
edited
Loading

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

@bedevere-app bedevere-app bot mentioned this pull request Apr 21, 2024
Closed
@nineteendo
Copy link
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.

@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 <berker.peksag@gmail.com>
Co-authored-by: Tahia Khan <tahia.khan@gmail.com>
Co-authored-by: Zachary Ware <zachary.ware@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@serhiy-storchaka
Copy link
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.

@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
33 checks passed
@nineteendo
Copy link
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

@vstinner
Copy link
Member

Nice enhancement!

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@ta1hia @bedevere-bot @berkerpeksag @zware @serhiy-storchaka @terryjreedy @nineteendo @vstinner @the-knights-who-say-ni @ezio-melotti
0