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 638: Typographic fixes #2368

Merged
merged 4 commits into from
Apr 29, 2022
Merged

PEP 638: Typographic fixes #2368

merged 4 commits into from
Apr 29, 2022

Conversation

davidfstr
Copy link
Contributor

@davidfstr davidfstr commented Feb 26, 2022

I'm excited about the potential introduction of Lisp-style syntactic macros into Python. 😁

Most of this PR is a set of comments on PEP 638, directed to the PEP author @markshannon, expressed as inline .rst comments.

Not sure if this is appropriate to actually merge, but I did want to pose various comments/questions to parts of PEP 638 inline and this seemed like a reasonable way to start.

Edit: This PR now only contains typographic fixes to PEP 638. I have moved discussion of more-substantial points to python-dev.

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.

Thanks much for your feedback.

In general, for format/textual fixes on the PEP source, its best to make them as a PR for the PEP author(s) to review (as you did here, for such). To note, as a PEP editor, I reviewed and second all of them.

There isn't an ideal way to ask for clarification on specific textual items, headers, etc. (which were a lot of your comments) that you aren't able to propose yourself, but instead of baking inline comments into the file (especially all in the same commit as the previous, and thus have to be manually edited out instead of simply dropped), you could consider make them as review comments in the PR (if you've modified nearby lines), make them in a regular reply comment on the same, or as commit comments on the original commit that added/modified the line in question (probably 5c7d3aa in this case).

Generally speaking, it is usually better to direct more substantive questions and comments (whcih it looked like at least a few of yours were) about the content and decisions made in the PEP to the PEP's designed Discussions-To thread, so it can be considered by the authors, reviewers (particularly the SC/PEP delegate) and the interested community. In this case, the PEP doesn't actually specify it, making that difficult (or even the date that it was posted in Post-History); however, we've recently clarified and improved our guidance on this following #2346 which implemented #2266 (and is further improved by several other merged and in-review PRs), which explicitly requests that authors link the actual discussion thread, so that interested readers don't have to dig through Google and guess at what the current one is (which I think might be this one.

I was already working on a PR to clarify in the Contributing Guide what contributions are particularly helpful here, and I can try to clarify this as well as part of that (once we discuss it).

pep-0638.rst Outdated
Comment on lines 9 to 10
..
What is the Discussions-To for this PEP? python-dev?

Copy link
Member

Choose a reason for hiding this comment

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

Per our recently revised guidance (as of after this PEP was last updated), the Discussions-To header should point directly to the actual current discussion thread, rather than just mentioning the venue. This one appears to be the latest I can find after some digging. There were also at least one tangential thread posted by other users on the Python Discourse.

@CAM-Gerlach CAM-Gerlach changed the title PEP 638: Leave many comments. Typographic fixes. PEP 638: Typographic fixes (and many comments) Feb 26, 2022
@davidfstr
Copy link
Contributor Author

Thanks for taking the time to point me in a better direction @CAM-Gerlach .

I’ll plan to revise this PR to only make typographic fixes and insert a presumed Discussions-To link. Then make it non-draft.

Then I will move my inline comments to the actual discussions thread.

@davidfstr davidfstr marked this pull request as ready for review February 27, 2022 14:49
@davidfstr davidfstr changed the title PEP 638: Typographic fixes (and many comments) PEP 638: Typographic fixes Feb 27, 2022
pep-0638.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.

LGTM pending @markshannon 's review. Thanks @davidfstr

pep-0638.rst Outdated Show resolved Hide resolved
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Ping again @markshannon.

These changes look uncontroversial.

@davidfstr
Copy link
Contributor Author

I haven't been able to reach @markshannon by any means in months. And this PR in particular has now been open for 2 months.

@CAM-Gerlach / @hugovk Any chance this set of minor revisions could be merged even without his explicit approval?

@AA-Turner
Copy link
Member

Rebased to do the new CLA check.

A

@AA-Turner AA-Turner merged commit 26d7a56 into python:main Apr 29, 2022
@AA-Turner
Copy link
Member

AA-Turner commented Apr 29, 2022

Thanks! (Edit: yes, at least three editors have approved and the edits were beneficial to the text)

A

@davidfstr
Copy link
Contributor Author

Thanks all!

Amusingly I actually ran into Mark Shannon in-person today at PyCon US. ^_^

@CAM-Gerlach
Copy link
Member

Ha, I figured you might since I saw both of you there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants