-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
PEP 723: Embedding metadata in single-file scripts #3264
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
Conversation
|
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? |
|
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__ *= *"""$(.+?)^"""$ | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
-
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. ↩
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
AA-Turner
left a comment
There was a problem hiding this 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
|
@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 |
AA-Turner
left a comment
There was a problem hiding this 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:
|
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. |
|
@ofek -- Final comment, please could you wrap lines to 79 characters? |
|
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 |
Please create the new thread for this revision of the PEP, add it to "Post-History", and replace "Discussions-To" with it. A |
AA-Turner
left a comment
There was a problem hiding this 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
pep-NNNN.rst), PR title (PEP 123: <Title of PEP>) andPEPheaderAuthororSponsor, and formally confirmed their approvalAuthor,Status(Draft),TypeandCreatedheaders filled out correctlyPEP-Delegate,Topic,RequiresandReplacesheaders completed if appropriate.github/CODEOWNERSfor the PEPPython-Versionset to valid (pre-beta) future Python version, if relevantDiscussions-ToandPost-History📚 Documentation preview 📚: https://pep-previews--3264.org.readthedocs.build/