GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — PEP 678: more rejected ideas by Zac-HD · Pull Request #2205 · python/peps

Conversation

@Zac-HD

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

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Zac-HD
Zac-HD requested a review from iritkatriel as a code owner December 27, 2021 11:41
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Dec 27, 2021
iritkatriel
iritkatriel reviewed Dec 27, 2021
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum
gvanrossum reviewed Dec 27, 2021
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Jan 10, 2022

Copy link
Copy Markdown
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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Zac-HD
Zac-HD force-pushed the exception-note branch from 9a118c2 to d95f7d9 Compare January 21, 2022 08:23
@Zac-HD

Zac-HD commented Jan 21, 2022

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel reviewed Jan 21, 2022
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel reviewed Jan 21, 2022
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel reviewed Jan 21, 2022
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

AA-Turner
AA-Turner requested changes Jan 21, 2022
View reviewed changes

@AA-Turner AA-Turner left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Some syntax suggestions and a couple of questions on wording
A

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Outdated
Comment on lines 291 to 292
Store notes in ``ExceptionGroup`` s
-----------------------------------

@AA-Turner AA-Turner Jan 21, 2022

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
CAM-Gerlach
CAM-Gerlach reviewed Jan 22, 2022
View reviewed changes

@CAM-Gerlach CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

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.

A few minor syntax fixes and a question for clarification

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Jan 24, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

encukou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel reviewed Jan 26, 2022
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Zac-HD
PEP 678: more rejected ideas
d122621
@Zac-HD
Zac-HD force-pushed the exception-note branch from 25ab9cd to ab1ca0f Compare January 26, 2022 09:15
@Zac-HD
Structure, footnotes, formatting
1b7442c
@Zac-HD
Zac-HD force-pushed the exception-note branch from ab1ca0f to 1b7442c Compare January 26, 2022 09:18
iritkatriel
iritkatriel reviewed Jan 26, 2022
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel reviewed Jan 26, 2022
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel
iritkatriel reviewed Jan 26, 2022
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel
Apply suggestions from code review
f88d006
iritkatriel
iritkatriel approved these changes Jan 26, 2022
View reviewed changes

@iritkatriel iritkatriel left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
JelleZijlstra
JelleZijlstra reviewed Jan 26, 2022
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

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.

Noticed two typos, I'll just commit the fixes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread pep-0678.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra added 2 commits January 26, 2022 06:25
@JelleZijlstra
Update pep-0678.rst
59fd727
@JelleZijlstra
Update pep-0678.rst
1ecc417
JelleZijlstra
JelleZijlstra approved these changes Jan 26, 2022
View reviewed changes
@iritkatriel
iritkatriel merged commit 854f676 into python:main Jan 26, 2022
@AA-Turner

AA-Turner commented Jan 26, 2022

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Jan 26, 2022

Copy link
Copy Markdown
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.

AA-Turner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CAM-Gerlach

CAM-Gerlach commented Jan 26, 2022

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Zac-HD

Zac-HD commented Jan 26, 2022

Copy link
Copy Markdown
Contributor Author

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

AA-Turner and CAM-Gerlach reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Zac-HD

Zac-HD commented Jan 26, 2022

Copy link
Copy Markdown
Contributor Author

@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 😅

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AA-Turner

AA-Turner commented Jan 26, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

CAM-Gerlach and Zac-HD reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland erlend-aasland mentioned this pull request May 13, 2022
pep8/greppable exception messages erlend-aasland/peps#1
Closed
@erlend-aasland erlend-aasland mentioned this pull request Jun 27, 2022
pep 687/mark as accepted erlend-aasland/peps#2
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum left review comments
@AA-Turner AA-Turner AA-Turner requested changes
@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments
@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@iritkatriel iritkatriel iritkatriel approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

CLA signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@Zac-HD @iritkatriel @AA-Turner @CAM-Gerlach @JelleZijlstra @gvanrossum @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.