Skip to content
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

PEP 704: Require virtual environments by default for installers #2964

Merged
merged 12 commits into from
Jan 25, 2023

Conversation

pradyunsg
Copy link
Member

Enforce the use of virtual environments and establish a conventional name for the virtual environment directory.

@pradyunsg pradyunsg requested a review from a team as a code owner January 16, 2023 23:40
@CAM-Gerlach CAM-Gerlach added the new-pep A new draft PEP submitted for initial review label Jan 17, 2023
@CAM-Gerlach
Copy link
Member

Looks like PEP 704 is available, so it's all yours.

@CAM-Gerlach CAM-Gerlach changed the title PEP 9999: Require virtual environments by default for installers PEP 704: Require virtual environments by default for installers Jan 17, 2023
Enforce the use of virtual environments and establish a conventional
name for the virtual environment directory.
pep-0704.rst Outdated Show resolved Hide resolved
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Basic requirements (all PEP Types)

  • File created from the latest PEP template
  • PEP has next available number, with filename (pep-NNNN.rst) and PEP header set accordingly
  • Title clearly, accurately and concisely describes the content in 79 characters or less
  • PEP, Title, Author, Status (Draft), Type and Created headers filled out correctly
  • Sponsor, PEP-Delegate, Topic, Requires and Replaces headers completed if appropriate
  • Required sections included
    • Abstract (first section)
    • Copyright (last section; exact wording from template required)
  • Code is well-formatted (PEP 7/PEP 8) and is in code blocks, with the right lexer names if non-Python
  • PEP builds with no warnings, pre-commit checks pass and content displays as intended in the rendered HTML
  • Authors/sponsor added to .github/CODEOWNERS for the PEP

Standards Track requirements

  • PEP topic discussed in a suitable venue with general agreement that a PEP is appropriate
  • Suggested sections included (unless not applicable)
    • Motivation
    • Rationale
    • Specification
    • Backwards Compatibility
    • Security Implications
    • How to Teach This
    • Reference Implementation
    • Rejected Ideas
    • Open Issues
  • Python-Version set to valid (pre-beta) future Python version Not needed for Packaging PEP that doesn't touch core Python
  • Any project stated in the PEP as supporting/endorsing/benefiting from it confirms such
  • Right before or after initial merging, PEP discussion thread created and linked to in Discussions-To and Post-History

General comments

  • When you're just referring to pip, the name of the package installer (as opposed to running the pip command on a CLI), I would suggest omitting the literal formatting, to be semantically correct, consistent with other names and pip's own docs, and avoid visual noise in the source and rendered output.
  • Just FYI, we've enabled Intersphinx against both the Python.org docs and packaging.python.org, so you can use all the normal directives you'd expect (:ref:, :term:, :module:, etc) to link against things like the PyPA glossary (which is rather outdated though I have some proposed updates in the works; for my working draft of PEP 639 I basically had to redefine most of the relevant terms myself with updated definitions).
  • I've included a handful of non-suggestion comments regarding the PEP clearly and precisely expressing what it intends to in terms of terminology and rationale, that may stray a bit into the gray area of content/subject matter expertise. If you feel any of them stray too far, I can defer them here and bring them up on the thread instead.

pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@pradyunsg
Copy link
Member Author

Are there any blocking concerns here, or can this be merged?

@JelleZijlstra
Copy link
Member

Did Brett (as the sponsor) approve the text of the PEP?

Also, you should add the PEP to CODEOWNERS (it's in CAM's checklist above; there's also an entry unchecked about headers but I am not sure what the issue is).

@pradyunsg
Copy link
Member Author

Yes: #2964 (comment)

CODEOWNERS added.

@JelleZijlstra
Copy link
Member

OK, then I think we can merge but I'll wait for a bit for @CAM-Gerlach to confirm.

@CAM-Gerlach
Copy link
Member

there's also an entry unchecked about headers but I am not sure what the issue is).

That was for confirming that the sponsor approved*; I saw Brett's comment and marked it as resolved, but forgot to update the checklist accordingly, sorry. Everything's checked now ✔️ ; there's just a handful of remaining small typos, textual fixes and syntax tweaks that either got missed from suggestions that were outdated/resolved for other reasons, on newly added material, or that I missed the first time around.

* In earlier versions of the checklist, I had a separate item for that, but Petr suggested merging it with the others which I went along with, which I'm now having second thoughts about...but I don't want to drag #2956 out even more since I already revised it several times.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

A handful of remaining small typos, textual fixes and syntax tweaks that either got missed from suggestions that were outdated/resolved for other reasons, on newly added material, or that I missed the first time around. Besides that, and in terms of the checklist, LGTM, thanks.

pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
pep-0704.rst Show resolved Hide resolved
pep-0704.rst Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
pradyunsg and others added 2 commits January 22, 2023 11:20
Co-authored-by: Zak Johnson <me@zakj.net>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@pradyunsg
Copy link
Member Author

Any blockers to landing this?

@JelleZijlstra JelleZijlstra merged commit 6c40d23 into python:main Jan 25, 2023
@pradyunsg pradyunsg deleted the require-venv branch January 25, 2023 14:16
Copy link

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Just a comment about that one. I really like that we have an opt-out now rather than opt-in (but it's good we have an explicit opt-out as well that is useful in many cases - for example in most container use-cases. Those are specific use-cases and having and opt-out possible is more than enough for those cases - especially for legacy use cases.

Also having a default convention for the venv usage is great as part of this PEP. This will make a number of use-cases simpler and less decisions to make and having implicit steps for activation of ~/.venv environment is a good one too.

@pradyunsg
Copy link
Member Author

Please don't comment on the PR, there's a discuss.python.org thread for this PEP which is where discussions should happen.

@potiuk
Copy link

potiuk commented Jan 27, 2023

Please don't comment on the PR, there's a discuss.python.org thread for this PEP which is where discussions should happen.

sure. I was not sure what the etiquette is but if you say so, I am happy to add this comment to the discourse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-pep A new draft PEP submitted for initial review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants