-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 501: Re-open and revise in consideration of PEP 701 #3047
Conversation
e5ab736
to
586afe7
Compare
It would be helpful to provide some important context when re-opening and making major changes to someone else's deferred PEP. In particular, before considering re-opening this PEP and reviewing this PR, it seems at least the following key information should be provided:
Thanks! |
Thanks; it would have been very helpful and germane to include in the original OP, and in the PEP itself. That answers most of my questions above, thanks. @nhumrich is it ready for our review, then? |
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.
Standard reminder: You can directly apply all the suggestions you want in one go with Files changed
-> Add to batch -> Commit
I need to sleep now and will follow up with more fixes on the rest later, but I added some suggestions and one larger comment on the headers and Abstract. I suggest batch-applying the suggestions first before making the larger changes recommended by the comment, to avoid them getting lost. Thanks.
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 again for tackling the update of this PEP to reflect the changes that have taken place since PEP 498 was accepted!
The minor comments I mentioned via email have been made inline, but I also added a few more substantive ones that didn't occur to me until either seeing @CAM-Gerlach's comments or simply reading it in this format rather than on my phone.
No fundamental changes, just cleaning up a few things that either I had missed back in the original PEP text, or else notes that made sense back before PEP 498 was accepted, but aren't relevant now.
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.
Standard reminder: You can directly apply all the suggestions you want in one go with Files changed
-> Add to batch -> Commit I suggest applying the suggestions first before considering the recommended reorganization changes.
Also, there appear to be a number of instances where what you now call "template literals" or "template strings" are still referred to as "interpolation templates", despite other instances being changed. You might want to consider whether those should be updated too for consistency.
Just to be clear, you don't need to (and shouldn't) apply all these suggestions manually. Instead, apply all the suggestions you want in one go by navigating to the Also, it looks like you mistakenly made these changes in the Do not proceed until AFTER this PR is merged!
|
Thanks for the thorough review @CAM-Gerlach! (as well as the recommendations on applying the change suggestions cleanly) The missing context is on me - I asked @nhumrich to make the PR so I could provide my own line-by-line review, but didn't consider how the PR would look to the PEP editors without knowing we had already been chatting about the update via email! If I had thought about it, I would have suggested making sure our prior email discussion was mentioned in the PR description. |
Another alternative approach to consider for any future such situations is suggesting the contributor make a PR on their fork instead for your initial review, and then iterating together there until it is ready to submit to the upstream PEPs repo for PEP editor review and publication. That way, you (and others you invite/are interested) can more easily iterate on the PEP and work on it together first to get it ready for review, and it avoids pinging all the PEP editors and the many other folks watching this repo before with every comment and change on the PR.
Yeah, the lack of a PR description, explanation, discussion links or other context made me concerned that this might be a PEP hijacking attempt (albeit a presumably unintentional one) 😂 , i.e. |
Co-authored-by: C.A.M. Gerlach <[email protected]> Co-authored-by: Nick Coghlan <[email protected]>
Co-authored-by: C.A.M. Gerlach <[email protected]>
* Added abstract * added __eq__ and __bool__ to TemplateLiteral * formated code * updated copyright
Thank you for all your suggestions and reviews. I believe I have resolved them, and am ready for another review. @ncoghlan @CAM-Gerlach |
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.
Looking pretty good, thanks! Just a handful of relatively minor followup suggestions, and one comment/request from my side.
Co-authored-by: C.A.M. Gerlach <[email protected]>
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.
(Merge conflict resolved) |
My apologies, this PR dropped off my radar until the publication of PEP 750 prompted me to re-read PEP 501 and wonder what became of @nhumrich's changes (only to realise I had never merged them). I will tidy up the lint checks and merge this, but then @nhumrich and I will need to consider whether we want to continue pursuing this, or whether we think it makes sense to withdraw it in favour of PEP 750. |
I think it might make more sense to withdraw and wait to see what happens from 750. 750 feels like it handles all the needs of 501, and the two together might be confusing. |
After reading the full text of PEP 750, I'm far from convinced that defining a new "string calling convention" (which is effectively what the PEP defines) is a good idea. However, there are a lot of ideas I do like in PEP 750, so I've started accumulating them in #3904 as potential amendments to PEP 501 (it's mostly stealing the |
As far as the PEP process goes, I expect the outcome will be that we'll end the process with one of the PEPs Rejected and the other one Accepted. As you say, there'd be no reason to do both. I mentioned withdrawing PEP 501 above because I expected to like the overall tagged strings proposal a lot more than I did before reading the full PEP, but it turned out to come with a lot more baggage than I expected. Since we can adopt all the good implementation bits wholesale while keeping the simpler surface syntax proposal, we may as well see if we can convince either the PEP 750 authors or the SC of the benefits of that approach :) |
📚 Documentation preview 📚: https://pep-previews--3047.org.readthedocs.build/