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 735: apply community feedback, including reversion of [project] table changes #3990

Merged
merged 10 commits into from
Oct 1, 2024

Conversation

sirosen
Copy link
Contributor

@sirosen sirosen commented Sep 23, 2024

This PR applies several changes in response to community feedback and my having been convinced by these arguments that modifications to the [project] table are a mistake within the scope of this PEP.

The commits here each addresses a separate piece of feedback:

  • Remove [project] changes from Dependency Groups.
    This is the largest change, removing text about changing the [project] table and making several adjustments for correctness in response to this.

  • Typing fix to PEP 735 reference implementation

  • PEP735: Add clarification on 'requirements.in'
    This adds a small blurb to "How to Teach This" on requirements.txt vs requirements.in.1

  • More strongly discourage overlapping extras
    This changes a SHOULD NOT to a MAY, thereby widening the PEP's allowance for tools to reject extras which conflict with dependency groups significantly.

  • Add PEP 735 note on direct extras usage
    An element of the feedback received was that using [project.optional-dependencies] does not require that the project defines a package if the toolchain allows for this usage.2

  • Add use-cases link to the motivation section
    More than one person found the Use Cases Appendix hard to find, so adding an extra link is hopefully benefiical.

There are two questions I have about whether or not to add text, noted in footnotes below.
In addition to basic copyediting, I'd appreciate any input on those two topic.
I'm still on the fence about those additions.

  • Change is either:
    • To a Draft PEP
    • To an Accepted or Final PEP, with Steering Council approval
    • To fix an editorial issue (markup, typo, link, header, etc)
  • PR title prefixed with PEP number (e.g. PEP 123: Summary of changes)

📚 Documentation preview 📚: https://pep-previews--3990.org.readthedocs.build/

Footnotes

  1. I'm not really convinced we need this text, or that it belongs in the PEP. Maybe I should go back and remove it?

  2. I'm also unsure about this element. It's making an allowance for usages which more or less seem to directly subvert PEP 621. I'm considering going back and removing this as well.

In response to feedback, and especially to the points about ecosystem
impacts and encouraging users to move package metadata out of the
current standard location, remove support for Dependency Group
Includes in the `[project]` table.

This is now listed with rationale in Rejected Ideas, and several
sections have been updated for correctness to fit this removal.
In "PEP 735: Dependency Groups", the defined semantics for a potential
name conflict between an extra and a group discouraged tools from
raising errors on such conflicts.

In the revised text, this is left completely open to implementers, and
it's more clearly likely and probable that some tools will error in
these cases. Users are still discouraged from creating this situation,
regardless of tool behaviors.
`uv` presented feedback that they allow for users to directly
reference content in the `[project]` table, even for non-package
projects. As a result, the text regarding extras as package metadata,
and the conflcit this presents with non-package projects, needs
clarification.

Simply add a section which calls this out explicitly and clarifies.
Some of the PEP feedback indicated that readers did not find the link
to the Use Cases section. Provide a second link in the Motivation
section, so that it is earlier in the document.
peps/pep-0735.rst Outdated Show resolved Hide resolved
This text was based on a misunderstanding of information shared in the
discourse forum. There is no need to keep it.
This is extending the PEP text for an ancillary point of confusion --
there are tool users for `uv` (and possibly `pip-tools`) who do not
understand that `requirements.in` is a convention that does not
represent a significantly distinct file-format.

Discussing the difference between these only lengthens the already
very-long PEP text, and does not add value vis-a-vis community
standards. Given that neither `requirements.in` nor `requirements.txt`
have clear specifications, other than "what `pip` accepts", keeping
with the older and more standardized naming keeps things shorter and
simpler.
@sirosen
Copy link
Contributor Author

sirosen commented Sep 24, 2024

I have pushed two updates here (in series) to remove the parts about which I was wavering. This PEP is already hugely long relative to the size of change it proposes, and IMO making it longer demands stronger justification than has been provided. Plus I got some of the details about uv wrong anyway, so pursuing concision will also help protect the accuracy of the text.

Per suggestion and with additional clarification.
@zanieb
Copy link

zanieb commented Sep 25, 2024

For what it's worth, I think adding more top-level fields to the pyproject.toml allowing declaration of dependencies is a huge change. It's small in syntax, but large in meaning :)

peps/pep-0735.rst Outdated Show resolved Hide resolved
peps/pep-0735.rst Outdated Show resolved Hide resolved
Move the sections on `[project]` includes and `[build-system]`
includes to Deferred Ideas, as an added section.

Restore the removed use case for this, as a subsection of one of these
Deferred Ideas.

Also, per suggestion, remove an unnecessary phrase ("defined below" in
a context where it is _immediately_ followed by the relevant
section).
@sirosen
Copy link
Contributor Author

sirosen commented Oct 1, 2024

I've applied the suggested edits, and took @ncoghlan's suggestion from the discourse to use Deferred Ideas. (I hadn't seen that section before, so I didn't know it was an option.)

@brettcannon brettcannon merged commit 79a41d1 into python:main Oct 1, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants