bpo-44388: Update venv EnvBuilder.ensure_directories() docs. #26663
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
Doc/library/venv.rst
Outdated
| Creates the environment directory and all necessary subdirectories that | ||
| don't already exist, and returns a context object. This is just a | ||
| holder for attributes (such as paths), for use by the other methods. | ||
| Any existing environment directories will remain unaffected as long as |
There was a problem hiding this comment.
I suggest changing the sentence to:
If the :class:EnvBuilder is created with the arg clear=True, contents of the environment directory will be cleared and then all necessary subdirectories will be recreated.
-- this describes the effect of the method more fully and given the description, there is no reason for the reader to think that existing env dir and subdirs will be affected in any way.
There was a problem hiding this comment.
I can do that. 👍
I was erring on the side of explicitness, because this function looks like it has ability to write to the directory structure, and the thought of that worried me. If it were a purely information-gathering API function I think I would've left that part of the sentence out.
Doc/library/venv.rst
Outdated
| exist already, as long as either ``clear`` or ``upgrade`` were | ||
| specified to allow operating on an existing environment directory. | ||
| Creates the environment directory and all necessary subdirectories that | ||
| don't already exist, and returns a context object. This is just a |
There was a problem hiding this comment.
Since we're updating this paragraph, another minor improvement to clarity may be as follows, because otherwise it's not very clear what 'this' refers to (e.g. the method, the instance, ..?). To me it's easier to understand if it's rephrased:
| don't already exist, and returns a context object. This is just a | |
| don't already exist, and returns a context object, which is just a |
There was a problem hiding this comment.
The only hesitation I have with this change is that the sentence gets pretty long with a bunch of commas / clauses.
Another simple fix which fix would just be to change This is just to This context object is just. A little explicit, but I like the fact that it results in two sentences with 2 comma-separated clauses instead of one long sentence with 4 comma-separated clauses.
There was a problem hiding this comment.
I agree, that was another option I've thought about. Both sound fine to me so if you prefer to keep 2 sentences, go ahead with that!
There was a problem hiding this comment.
I did the two-sentence version and removed a comma after the parenthetical phrase that seemed superfluous to me.
|
I've added one other small comment. With that change, the patch looks good to me. @vsajip can you also take a look? |
|
Sorry, I can't merge this PR. Reason: |
|
@itsayellow: Status check is done, and it's a success ✅ . |
Ref: https://bugs.python.org/issue44388
This changes the documentation for
EnvBuilder.ensure_directories(env_dir)to match the actual behavior of that API call.In particular,
ensure_directories()is not affected by the state of theupgradeattribute, and will not cause an error to have existing directories whether or not theclearattribute is set.This documentation change I believe should be valid to all python versions back to 3.6.
https://bugs.python.org/issue44388
Automerge-Triggered-By: GH:vsajip