FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-44347: [doc] clarify meaning of shutil.copytree's dirs_exist_ok kwarg by jdevries3133 · Pull Request #26643 · python/cpython · GitHub

Repository navigation

bpo-44347: [doc] clarify meaning of shutil.copytree's dirs_exist_ok kwarg - #26643

Closed
jdevries3133 wants to merge 70 commits into
python:mainfrom
jdevries3133:bpo-44347
Closed

jdevries3133 wants to merge 70 commits into
python:mainfrom
jdevries3133:bpo-44347

Conversation

jdevries3133 commented Jun 10, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor
  • It now mirrors the documentation for os.makedirs
  • Docstring in python source file was correspondingly updated

https://bugs.python.org/issue44347

https://bugs.python.org/issue44347

…kwarg

* It now mirrors the documentation for os.makedirs
* Docstring in python source file was correspondingly updated

jdevries3133 commented Jun 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@tvogel, continuing our conversation from now-closed #26634...

I agree, that should be mentioned and I added more detail for that. Additionally, I moved everything about dirs_exist_ok down to the bottom of the documentation and the docstring to match the order of arguments in the function signature.

Key Sentence Added

Furthermore, any parent directory in the path to dst will be created if they do not exist when the dirs_exist_ok flag is True.

nanjekyejoannah left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for your contribution @jdevries3133, can you please fix the errors reported in the CI?

Copy link
Copy Markdown
Contributor Author

@nanjekyejoannah I fixed it. Thank you for your patience!

Comment thread Doc/library/shutil.rst Outdated
that supports the same signature (like :func:`~shutil.copy`) can be used.

If *dirs_exist_ok* is false (the default), a :exc:`FileExistsError` is
raised if *dst* already exists. Conversely, *dst* as well as any any parent

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this phrasing is a bit unclear. 'conversely' implies that the following is opposite to how the default value works. In other words, I would read this sentence as saying that with dirs_exist_ok=False, dirs along the path to dst, as well as dst itself, will not be created.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, you are totally correct; I've made it more confusing. I changed it to be much more explicit.

FFY00 added the docs Documentation in the Doc dir label Jun 11, 2021

jdevries3133 commented Jun 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I think the failing test is related to bpo-38323. I only made changes to docs / docstrings, and make check html SPHINXOPTS="-q -W -j4" was happy with me.

zooba and others added 14 commits June 19, 2021 11:00
In particular, when running with tk8.6.8, as in PSF 3.9. 

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
…-26606)

Change the behaviour of `math.pow(0.0, -math.inf)` and `math.pow(-0.0, -math.inf)` to return positive infinity instead of raising `ValueError`. This makes `math.pow` consistent with the built-in `pow` (and the `**` operator) for this particular special case, and brings the `math.pow` special-case handling into compliance with IEEE 754.
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Signed-off-by: Christian Heimes <christian@python.org>
…results (pythonGH-26715)

* Simplify the count_vowels example
* Hits and misses are fetched while a lock is held
* Add note that references are kept for arguments and return values
* Clarify behavior when *typed* is false.
…-26638)

* Add specializations of LOAD_GLOBAL.

* Add more stats.

* Remove old opcache; it is no longer used.

* Add NEWS

jdevries3133 commented Jun 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@akulakov Ah, I see what you are saying; however, I'm still happy with what I added because:

  1. dirs_exist_ok is a good name for the parameter here; simply exist_ok would not be a better name. In os.makedirs(exist_ok=False), it is clear to the user that this switch only applies to directories (because makedirs itself obviously only touches directories). shutil.copytree's function name itself does not provide the same context, though, so it is worthwhile for the parameter to be more explicit.
  2. Referencing os.makedirs will only help users who already know how that function behaves. For newcomers, I don't want to give only a breadcrumb and send them on a scavenger hunt.

All that being said, I did add one sentence about the relationship between dirs_exist_ok and os.makedirs's exist_ok parameters.

github-actions Bot commented Aug 1, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Aug 1, 2021
Comment thread Doc/library/shutil.rst Outdated
jdevries3133 marked this pull request as draft April 3, 2022 00:22
* remove the note which is excessive
* remove all changes to `Lib/shutil.py`, pending approval of the new
  docs, I'll re-add revisions to shutil.copytree's docstring.
* revise first paragraph

Copy link
Copy Markdown
Contributor Author

@JelleZijlstra I revised the PR. If it looks good, I will add the paragraph about dirs_exist_ok to the docstring in Lib/shutil.py, and then this should be good to go.

I deleted 80% of the PR and rewrote the rest, so please review as if this is all new.

JelleZijlstra self-requested a review April 8, 2022 22:10

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks!

Comment thread Doc/library/shutil.rst Outdated
Comment thread Doc/library/shutil.rst Outdated

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The text looks good! Can you update the docstring too and undraft the PR?

ghost commented Apr 10, 2022

Copy link
Copy Markdown

Commit authors are required to sign the Contributor License Agreement.

jdevries3133 marked this pull request as ready for review April 10, 2022 15:31

Copy link
Copy Markdown
Contributor Author

@JelleZijlstra OK, good to go. Not sure why the CLA bot is complaining, I clicked the link and re-signed but it still says I'm unsigned. I'm guessing it's just a GitHub migration glitch, I've made a handful of contributions in the past and did sign the CLA.

Copy link
Copy Markdown
Contributor Author

CI failure is a fluke:

E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/main/c/ccache/ccache_3.7.7-1_amd64.deb  Connection failed [IP: 52.147.219.192 80]
E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?
Error: Process completed with exit code 100.

Can you trigger a re-run?

Copy link
Copy Markdown
Member

CI failure is a fluke:

E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/main/c/ccache/ccache_3.7.7-1_amd64.deb  Connection failed [IP: 52.147.219.192 80]
E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?
Error: Process completed with exit code 100.

Can you trigger a re-run?

Probably related to python/the-knights-who-say-ni#318

Copy link
Copy Markdown
Member

I wonder if the CLA bot is unhappy because of all the random commits that were added to the branch earlier.

Copy link
Copy Markdown
Member

I wonder if the CLA bot is unhappy because of all the random commits that were added to the branch earlier

Perhaps a force-push would sort that out?

Copy link
Copy Markdown
Contributor Author

I just created a new branch and a new PR with a single commit:

#91434

Use it if that's helpful, close it if not :)

Copy link
Copy Markdown
Member

Merged in #91434.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting merge docs Documentation in the Doc dir stale Stale PR or inactive for long period of time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.


Back | FazBrowse Home | New Git URL