PEP 727: Review #3316
PEP 727: Review #3316
Conversation
There was a problem hiding this comment.
Thanks a lot for this!
And special thanks for being able to put on the hat of PEP editor and give gentle feedback purely about the PEP, isolated from other possible concerns (personal as a developer, or as a Sphinx maintainer). Hats off to you. 🎩
Now, about the process, should this be merged, and then I fill the missing parts in another PR? Should I create a new PR including these commits?
There are additional changes coming, I'll make it a single Doc class, with a single positional-only parameter, and that's it.
And maybe some of the parts that I thought would be useful "for completeness" would be better removed, like the complex scenarios combining multiple nested Annotated.
Also something about how other languages do.
I was thinking, it would probably be good to have, in the "motivation section, a simple bullet point list with all the things this would "solve" that aren't currently solved by existing alternatives. A lot of the criticism suggesting existing or proposed alternatives keeps missing some of those points.
Should I wait to get this merged first and then do that part?
|
Yes, probably easiest to merge this first (Adam or I can merge it once you approve), then make your changes. |
All part of the job! Thank you for the kind words, though, very much appreciated.
Up to you really! I'd suggest merging this PR is easier -- I added the missing sections in a reST comment to act more as bookmarks, but they could be removed before merging if you'd like. A |
|
Thanks @AA-Turner! I think it's fine to leave the placeholders and I'll edit them. I added a couple of comments above, on the conversation about errors and MUST/SHOULD. It would also be fine to leave it as is and then I could edit on top, I think (?) |
|
Hello @AA-Turner! I realized we didn't decide how to proceed. And now I'm thinking that maybe we are all here waiting for each other... will you update this PR with those small comments or should we merge it and then I change things on top? If it's just that you haven't had time, totally get it... but I imagined we could be waiting for each other in a deadlock and you're actually waiting for me to hit "resolve" to merge this and continue on top... Let me know what works best for you. I'll hit "resolve" now in case that was the plan and you can merge. And then I can add all the recent updates on top (including no |
|
Ah, wait, what am I saying 😂 ...I can't hit resolve, I don't own this repo, I didn't start this PR, and I didn't start that comment. Anyway, let me know what's the best way to proceed. 🤓 |
|
@tiangolo if you approve this PR I'll merge it with the superpowers vested in me. |
Sorry Sebastián, a case of crossed wires! I didn't realise you were waiting for me -- please do what you want with this PR (merging it as Jelle suggests may be most expedient) -- I'll continue to review new changes. I've been meaning to reply to the Discourse thread, so sorry for that too -- thank you for your patience! A |
There was a problem hiding this comment.
Awesome, thank you both! 🚀
Yep, @JelleZijlstra please go ahead and merge it, thank you!
|
Thank you both! |
PEP 123: Summary of changes)cc: @tiangolo
Each commit addresses a distinct issue. The last 5 re-write various things I was unclear on or I thought could do with an edit -- I've also added the three missing headers from this PEP for filling in later.
As in the earlier review, my thoughts remain that the "Additional Scenarios" outlined should just be banned.
Feel free to ask for clarification on any of the suggested changes.
(I'll submit a PR to wrap to 79 columns per PEP-1 after this one, as that's a purely editorial change.)
A
📚 Documentation preview 📚: https://pep-previews--3316.org.readthedocs.build/