Skip to content

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

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.

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

Closed
wants to merge 70 commits into from

Conversation

jdevries3133
Copy link
Contributor

@jdevries3133 jdevries3133 commented Jun 10, 2021

  • 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
Copy link
Contributor Author

jdevries3133 commented Jun 10, 2021

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

Copy link
Contributor

@nanjekyejoannah nanjekyejoannah left a comment

Choose a reason for hiding this comment

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

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

@jdevries3133
Copy link
Contributor Author

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

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
Contributor

Choose a reason for hiding this comment

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

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
Contributor Author

Choose a reason for hiding this comment

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

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

@FFY00 FFY00 added the docs Documentation in the Doc dir label Jun 11, 2021
@jdevries3133
Copy link
Contributor Author

jdevries3133 commented Jun 11, 2021

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 <[email protected]>
…-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.
Greatly reduce pow() overhead for small exponents.
…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
Copy link
Contributor Author

jdevries3133 commented Jun 21, 2021

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

github-actions bot commented Aug 1, 2021

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

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Aug 1, 2021
@jdevries3133 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
@jdevries3133
Copy link
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 JelleZijlstra self-requested a review April 8, 2022 22:10
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

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

@ghost
Copy link

ghost commented Apr 10, 2022

Commit authors are required to sign the Contributor License Agreement.
CLA not signed

@jdevries3133 jdevries3133 marked this pull request as ready for review April 10, 2022 15:31
@jdevries3133
Copy link
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.

@jdevries3133
Copy link
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?

@AlexWaygood
Copy link
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

@JelleZijlstra
Copy link
Member

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

@AlexWaygood
Copy link
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?

@jdevries3133
Copy link
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 :)

@JelleZijlstra
Copy link
Member

Merged in #91434.

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.