peps icon indicating copy to clipboard operation
peps copied to clipboard

PEP 501: Re-open and revise in consideration of PEP 701

Open nhumrich opened this issue 2 years ago • 9 comments
trafficstars


:books: Documentation preview :books:: https://pep-previews--3047.org.readthedocs.build/

nhumrich avatar Mar 09 '23 04:03 nhumrich

All commit authors signed the Contributor License Agreement.
CLA signed

cpython-cla-bot[bot] avatar Mar 09 '23 04:03 cpython-cla-bot[bot]

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:

  • Confirmation from @ncoghlan that he's okay with you adopting and re-writing his PEP

  • Link to a discussion thread on, e.g. the Ideas or Core Development Discourse categories or mailing lists where this revised proposal was posted and discussed and there was at least some consensus in favor of going ahead with a PEP

  • At least a brief rationale for why it would be better to overhaul this existing PEP with major changes, as opposed to proposing a new fresh PEP with your own ideas that references the old one as direct inspiration—per PEP 1:

    Occasionally, a Deferred or even a Withdrawn PEP may be resurrected with major updates, but it is often better to just propose a new one.

Thanks!

CAM-Gerlach avatar Mar 09 '23 08:03 CAM-Gerlach

For reference: https://discuss.python.org/t/pep-501-reopen-general-purpose-string-template-literals/24625

hugovk avatar Mar 09 '23 22:03 hugovk

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?

CAM-Gerlach avatar Mar 10 '23 06:03 CAM-Gerlach

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 Files changed tab clicking Add to batch on each suggestion, and once you've added all of them, clicking Commit**, entering a commit message and committing. Alternatively, we can do it for you, if you prefer.

:warning: Important :warning: : You should apply our suggestions first before considering the recommended reorganization changes, or they will likely get out of date otherwise. Also, make sure to pull your remote branch into your local one first after applying the suggestions and before making any additional local changes.

Also, it looks like you mistakenly made these changes in the main branch of your fork rather than first creating a topic branch, which will lead to problems for you down the road on future PRs. You SHOULD NOT do anything about this now, as it will invalidate this PR and all our review suggestions, but ONLY AFTER THIS PR IS MERGED you can fix this with the following options:

Do not proceed until AFTER this PR is merged!
  • Delete your fork and recreate it, or

  • Do a bit of git surgery

    git switch master
    git fetch --all
    git reset --hard upstream/master
    git push -f origin master
    

CAM-Gerlach avatar Mar 11 '23 09:03 CAM-Gerlach

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.

ncoghlan avatar Mar 11 '23 12:03 ncoghlan

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!

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.

If I had thought about it, I would have suggested making sure our prior email discussion was mentioned in the PR description.

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) :joy: , i.e.

image

CAM-Gerlach avatar Mar 11 '23 13:03 CAM-Gerlach

Thank you for all your suggestions and reviews. I believe I have resolved them, and am ready for another review. @ncoghlan @CAM-Gerlach Please let me know if I missed anything.

nhumrich avatar Mar 14 '23 20:03 nhumrich

(Merge conflict resolved)

hugovk avatar Nov 04 '23 20:11 hugovk

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.

ncoghlan avatar Aug 12 '24 15:08 ncoghlan

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. Perhaps we hold off and await 750, and if 750 is rejected, we push 501? What are your thoughts?

nhumrich avatar Aug 12 '24 19:08 nhumrich

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 https://github.com/python/peps/issues/3904 as potential amendments to PEP 501 (it's mostly stealing the Decoded and Interpolation protocols outright, but also some novel ideas prompted by the lazy-vs-eager evaluation discussion).

ncoghlan avatar Aug 14 '24 04:08 ncoghlan

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 :)

ncoghlan avatar Aug 14 '24 04:08 ncoghlan