-
-
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 678: Update proposal with .add_note()
and acknowledgements section
#2331
Conversation
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.
Looks good, a few comments.
.add_note()
and acknowledgements section.add_note()
and acknowledgements section
Currently working on reviewing this, thanks. I will also have fixes for the issues causing the build failures on both rendering systems. |
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.
Just a few minor suggestions to fix the syntax issues causing build failures on both rendering systems, resolve a few grammar problems, improve a bit of syntax and refine a few of @iritkatriel 's suggestions from a textual perspective.
As always, for any you'd like to apply, I suggest using the Add to batch
button in the Files
tab and then committing with the Commit
button at the top. Thanks!
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
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.
LGTM
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.
LGTM, thanks @Zac-HD !
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.
Looks good, I'll keep my reservations about the proposed API to the Discuss thread.
Just replied over there; I do anticipate another PR before we go back to the Steering Council. (the discussions will continue until morale improves!) |
I do like your latest proposal :) |
The process is definitely producing a better proposal, and I'll also be happy when I'm done - mostly because I want to use it 😉 |
…ion (python#2331) Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
There is a cost to adding another key to BaseException’s dict. In any case, I don’t think this point needs to be resolved before resubmitting to the SC. It is very minor. |
Without recapping the diff, further discussion of translating messages on discuss.python.org prompted this updated design, which handles multiple notes by way of representing a sequence, rather than concatenating strings.
It is also, while I do anticipate further expansion, high time for an acknowledgements section.