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

feat: support PEP 723 run requirements #1100

Merged
merged 5 commits into from
Nov 30, 2023

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Nov 3, 2023

  • I have added an entry to docs/changelog.md

Summary of changes

Moved the unreleased feature from the rejected PEP 722 syntax to the (provisionally accepted a few moments ago in python/peps#3505) PEP 723 syntax.

Test plan

Tested by running

nox -s tests-3.11

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii marked this pull request as draft November 3, 2023 20:41
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii marked this pull request as ready for review November 3, 2023 22:57
@pradyunsg
Copy link
Member

👋 I would request that y'all hold off on merging this until there's agreement on what exactly the provisional status for the PEP means, in the context of user-facing tooling implementing the described functionality.

I've asked about what tooling is supposed to do right now in the acceptance thread for the PEP, because what they're supposed to do seems to be ambigous/unclear to me, and I think we should make the call explicitly to implement something that we might rip out later.

Comment on lines +329 to +331
PEP723 = re.compile(
r"(?m)^# /// (?P<type>[a-zA-Z0-9-]+)$\s(?P<content>(^#(| .*)$\s)+)^# ///$"
)
Copy link
Member

Choose a reason for hiding this comment

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

A comment on the regex pattern (and/or maybe using re.VERBOSE) would be a good idea

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'd say comment as long as we match the PEP's suggested regex.

name = "pyproject"

# Windows is currently getting un-normalized line endings, so normalize
content = content.replace("\r\n", "\n")
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 needed? From what I can tell not normalising would simply produce some harmless empty lines that do not match anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a newline is multiple multiple chars, then it doesn't match the regex. I pre-process this so I can use exactly the regex in the PEP. Another option would be to modify the regex to handle all possible new lines and only that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(The CI was breaking on Windows otherwise ;) )

@henryiii
Copy link
Contributor Author

henryiii commented Nov 6, 2023

explicitly to implement something that we might rip out later.

To be clear, PEP 722 (rejected) is already implemented in main, and this is just moving that unreleased code to PEP 723 (provisionally accepted). IMO, if this exists at all, it better in the provisionally accepted form rather than the rejected form.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@gaborbernat gaborbernat merged commit 60c5f24 into pypa:main Nov 30, 2023
0 of 8 checks passed
@gaborbernat
Copy link
Contributor

To be clear, PEP 722 (rejected) is already implemented in main, and this is just moving that unreleased code to PEP 723 (provisionally accepted). IMO, if this exists at all, it better in the provisionally accepted form rather than the rejected form.

Merged this because I agree here more with @henryiii than the concerns raised by Pradyun.

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.

4 participants