Skip to content

Conversation

@ofek
Copy link
Contributor

@ofek ofek commented Aug 4, 2023

  • Read and followed PEP 1 & PEP 12
  • File created from the latest PEP template
  • PEP has next available number, & set in filename (pep-NNNN.rst), PR title (PEP 123: <Title of PEP>) and PEP header
  • Title clearly, accurately and concisely describes the content in 79 characters or less
  • Core dev/PEP editor listed as Author or Sponsor, and formally confirmed their approval
  • Author, Status (Draft), Type and Created headers filled out correctly
  • 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
  • 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, if relevant
  • Any project stated in the PEP as supporting/endorsing/benefiting from the PEP formally confirmed such
  • Right before or after initial merging, PEP discussion thread created and linked to in Discussions-To and Post-History

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

@ofek ofek requested a review from a team as a code owner August 4, 2023 06:22
@hugovk hugovk added the new-pep A new draft PEP submitted for initial review label Aug 4, 2023
@hugovk
Copy link
Member

hugovk commented Aug 4, 2023

723 is good, thanks.

Please could you copy the relevant checklist from https://github.com/python/peps/blob/main/.github/PULL_REQUEST_TEMPLATE/Add%20a%20new%20PEP.md into the top message and check off those that are done?

@hugovk
Copy link
Member

hugovk commented Aug 4, 2023

This PEP extends PEP-722 with an alternative format.

Does it include 722's metadata block format in addition to TOML, or is only TOML proposed?

Let's make that clear.

This regular expression may be used to parse the metadata::

(?ms)^__pyproject__ *= *"""$(.+?)^"""$

Copy link
Member

Choose a reason for hiding this comment

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

Is this regular expression the specification of the format, or just an example? Because it does not handle the full range of possibilities here. What about

__pyproject__ = """\
    [project]
    ...
"""

While the backslash isn't necessary here, I regularly use multi-line strings in a context where a leading newline would be an error, so I tend to add the backslash without thinking. So I'd be surprised to find that a spec that says "a multi-line double-quoted string containing a valid TOML document" disallows this possibility.

There's also the possibility of people using doubled backslashes to include a backslash in the TOML.

This is fundamentally the issue with "use Python notation". Are you using Python, or are you using some subset? If the latter, then you need to define it explicitly, or someone will insist that the standard says you must support some obscure facet of Python syntax that you didn't even consider.

Copy link

@abravalheri abravalheri Aug 4, 2023

Choose a reason for hiding this comment

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

Probably, I should have posted what I wrote in the discourse thread directly here.

The following are other examples (non-exhaustive) of syntaxes that (to my understanding, and I might be wrong) seem to be permitted by the current state of the draft, are valid Python code, result in a string value (after Python's syntax interpretation) that is valid TOML, but might break the reference implementation:

__pyproject__ = """
[project]
readme.text = \"""
Hello, this is my awesome single-file utility script.
Try running::

    pipx run myfile.py
\"""
dependencies = ['tomli; python_version < "3.11"']
"""

__pyproject__ = """
project.dependencies = """ """\
['tomli; python_version < "3.11"']
"""

__pyproject__ = (
   """
   project.dependencies = ['tomli; python_version < "3.11"']
   """
)  # Auto-formatters sometimes add parenthesis...

...

This is related to the previous comment.
The parsing problems might be addressed by restricting the allowed syntax1 or improving the reference implementation to be "more Python-aware".

Footnotes

  1. Restricting the allowed syntax allows implementers to confidently reply "Please don't do that" to users requests, but it comes with the drawback of having to educate users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added:

In circumstances where there is a discrepancy between the regular expression and the text specification, the text specification takes precedence.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, but in which case the text specification isn't clear enough (IMO) to write a parser for it. Note that I'm thinking about this from the perspective of potentially having to implement this for pipx, not as the author of a competing PEP.

And to be clear, if the PEP says that there is the potential for a discrepancy between the suggested regex and the formal spec, I'd personally avoid using the regex, because I prefer correctness over convenience. So this is a real issue for me.

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

In addition to what Hugo & Paul have said, I think this PEP should aim to stand alone -- any content refereneced from PEP-722 should be incorporated into this one.

Editorially, I also think that the Specification section needs tightening up, and ideally some of the differentiating factors of this PEP vs PEP-722 can be placed into your Motivation and Rationale sections.

A

@AA-Turner
Copy link
Member

AA-Turner commented Aug 6, 2023

@ofek -- please do not rebase / force push updates to your PEP, it doesn't play nicely with the GitHub review interface. Please may you instead merge & push -- we squash-merge on commit so history in your branch doesn't need to be kept tidy.

A

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

I've gone through and marked all the comments I believe you've addressed as 'Resolved'; two re-issued comments:

@ofek
Copy link
Contributor Author

ofek commented Aug 6, 2023

Thanks! I think I have addressed all of the preliminary feedback, please let me know if this is ready to be merged for the subsequent proper review/discussion.

I won't force push again, I only do so for commits that address feedback but never between reviews.

@AA-Turner
Copy link
Member

@ofek -- Final comment, please could you wrap lines to 79 characters?

@ofek
Copy link
Contributor Author

ofek commented Aug 6, 2023

Is there a tool for that? It seems other PEPs don't adhere to that for example on line 77 of PEP 722 there are 80 characters.

@AA-Turner
Copy link
Member

Is there a tool for that? It seems other PEPs don't adhere to that for example on line 77 of PEP 722 there are 80 characters.

I'm happy to handle it, will push a commit. It's PEP 12 guidance, but we don't enforce it via lint checks.

A

@AA-Turner
Copy link
Member

AA-Turner commented Aug 6, 2023

Right before or after initial merging, PEP discussion thread created and linked to in Discussions-To and Post-History

Please create the new thread for this revision of the PEP, add it to "Post-History", and replace "Discussions-To" with it.

A

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Editorially, all good.

A

@AA-Turner AA-Turner merged commit 8da14ef into python:main Aug 6, 2023
@ofek ofek deleted the pep-723 branch August 6, 2023 18:17
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.

7 participants