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 678: more rejected ideas #2205

Merged
merged 5 commits into from
Jan 26, 2022
Merged

PEP 678: more rejected ideas #2205

merged 5 commits into from
Jan 26, 2022

Conversation

Zac-HD
Copy link
Contributor

@Zac-HD Zac-HD commented Dec 27, 2021

I've added a few sections addressing rejected ideas from this discussion on Reddit, and laid out my position about non-str notes in response to @warsaw's #2201 (comment).

My own design intuition is pretty clearly against non-string notes... but my respect for the voice of experience is also strong. This draft of the PEP therefore describes the current implementation (string-or-None), but I'm open to revising that based on python-dev discussion (planning to post early in the new year).

pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
@iritkatriel
Copy link
Member

I just noticed that the exception chaining PEP (3134) links to this python-dev post:
https://mail.python.org/pipermail/python-dev/2003-January/032492.html
which suggested to make it possible to append to the exception message instead of overwrite it (when raising). The reply suggests to attach the previous exception as the cause of the new one instead, which was recognised as the better approach.

In terms of guidelines for when to use chaining and when to use notes, maybe it goes back to this - are we about to re-raise the exception (then we chain) or are we doing something else with it, like storing it for later or grouping it in an ExceptionGroup, and then we add a note.

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Jan 21, 2022

OK, I'm finished this PR and - unless anyone has further feedback here - will be ready to post to python-dev early next week 😁

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

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Some syntax suggestions and a couple of questions on wording
A

pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated
Comment on lines 291 to 292
Store notes in ``ExceptionGroup`` s
-----------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above, if the space is required please could you escape it

Suggested change
Store notes in ``ExceptionGroup`` s
-----------------------------------
Store notes in ``ExceptionGroup``\ s
------------------------------------

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.

A few minor syntax fixes and a question for clarification

pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
@iritkatriel
Copy link
Member

iritkatriel commented Jan 24, 2022

unless anyone has further feedback here - will be ready to post to python-dev early next week 😁

I imagine the SC will appreciate this being submitted ahead of the PEP rush leading up to the feature freeze.

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

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

LGTM.

@AA-Turner @CAM-Gerlach Any further comments?

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Noticed two typos, I'll just commit the fixes

pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
@iritkatriel iritkatriel merged commit 854f676 into python:main Jan 26, 2022
@AA-Turner
Copy link
Member

Sorry I didn't get a chance to re-review -- looks good though, thanks @Zac-HD & @iritkatriel!
A

@iritkatriel
Copy link
Member

@AA-Turner Thank you for your review. I went ahead and merged so @Zac-HD can post it, but it's still a draft so we can fix things in future PRs. Let us know if you see anything.

@CAM-Gerlach
Copy link
Member

Looks good as well, thanks! Per #2130 , you could consider converting the references from footnotes to actual link targets and just directly linking the appropriate text in the body (aside from the one that is an actual footnotes), to make them much easier to follow for readers. E.g. subject to `breakage without warning`_ and in the References/Footnotes section,

.. _breakage without warning: https://example.com

Or, you could give the link target a short name instead, e.g. breakage, and reference it like this: subject to `breakage without warning <breakage_>`_.

But, to be clear, you don't have to do that if you don't want, if you feel it creates too much churn. Or I'm happy to open a PR for you.

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Jan 26, 2022

🙏 Thanks so much to everyone for you feedback and fixes - the document is so much better than it would be without you!

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Jan 26, 2022

@CAM-Gerlach - I've really just imitated other PEPs here; my impression is that the use of footnotes is mostly for ease of reading in e.g. mail clients which don't support HTML or RST markup. That does seem like more of a historical concern, but I'm hesitant to break with tradition in my first PEP 😅

@AA-Turner
Copy link
Member

AA-Turner commented Jan 26, 2022

use of footnotes is mostly for ease of reading in e.g. mail clients which don't support HTML or RST markup. That does seem like more of a historical concern

I think ease of reading on the web is far more important -- a lot of recent edits to various PEPs have included inlining links. Appreciate the concerns, though!

A

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