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: Enriching Exceptions with Notes by Zac-HD · Pull Request #2201 · python/peps

Conversation

@Zac-HD

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

Copy link
Copy Markdown
Contributor

@iritkatriel mentioned to me that the addition of BaseException.__note__ is a change to a builtin and therefore - while it's already in CPython main - requires some further discussion. I've therefore drafted a PEP to explain the changes, motivation, and rejected alternatives.

I'm generally happy to help out with this around my holiday plans and amend HypothesisWorks/hypothesis#3191 to reflect any necessary changes, but would appreciate any advice people have - I've read the process docs but it's still my first PEP 😄

Sorry, something went wrong.

Uh oh!

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

timsavage reacted with thumbs up emoji iritkatriel and erlend-aasland reacted with rocket emoji
All reactions
  • 👍 1 reaction
  • 🚀 2 reactions
Zac-HD added 2 commits December 20, 2021 19:07
@Zac-HD
PEP 9999: Enriching Exceptions with Notes
00dc59a
Draft by request of @iritkatriel describing the new BaseException.__note__ we co-designed.
@Zac-HD
PEP 9999: draft review updates
b4c2657
Thanks again to Irit for helpful comments :-)
@Zac-HD
Zac-HD requested a review from a team as a code owner December 20, 2021 12:22
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Dec 20, 2021
iritkatriel
iritkatriel reviewed Dec 20, 2021
View reviewed changes
Comment thread pep-9999.rst Resolved

Uh oh!

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

@AlexWaygood AlexWaygood mentioned this pull request Dec 20, 2021
Add __note__ to BaseException in Python 3.11 python/typeshed#6633
Merged
gvanrossum
gvanrossum reviewed Dec 20, 2021
View reviewed changes

@gvanrossum gvanrossum 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.

Please address the alternative proposed by Nathaniel, who suggested to attach the note to individual traceback entries; PyTracebackObject instances would get a new field ‘note’.

Otherwise LG.

Sorry, something went wrong.

Uh oh!

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

All reactions
JelleZijlstra
JelleZijlstra requested changes Dec 20, 2021
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.

A couple of copyediting suggestions

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread .github/CODEOWNERS Resolved

Uh oh!

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

Comment thread pep-9999.rst Resolved

Uh oh!

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

Comment thread pep-9999.rst Resolved

Uh oh!

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

Comment thread pep-9999.rst Resolved

Uh oh!

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

Comment thread pep-9999.rst Resolved

Uh oh!

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

Comment thread pep-9999.rst Resolved

Uh oh!

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

Comment thread pep-9999.rst Resolved

Uh oh!

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

Comment thread pep-9999.rst Resolved

Uh oh!

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

Comment thread pep-9999.rst Resolved

Uh oh!

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

@iritkatriel

iritkatriel commented Dec 20, 2021

Copy link
Copy Markdown
Member

Please address the alternative proposed by Nathaniel, who suggested to attach the note to individual traceback entries; PyTracebackObject instances would get a new field ‘note’.

Otherwise LG.

Was this proposal made in public yet? (Otherwise Zac probably doesn't know about it).

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Dec 20, 2021 via email

Copy link
Copy Markdown
Member
Here's a paraphrase: https://docs.google.com/document/d/1ptNdtAe4vywW2jldOxbT0iYgRxhA0VjfCAVcYXohEH0/edit
All reactions

Sorry, something went wrong.

Uh oh!

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

@brettcannon

brettcannon commented Dec 20, 2021

Copy link
Copy Markdown
Member

Please take PEP 678 as your number (it should simply be the next available number in case I missed a PR w/ a proposed PEP that's already claimed that number).

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.

@iritkatriel

iritkatriel commented Dec 20, 2021

Copy link
Copy Markdown
Member

Here's a paraphrase: https://docs.google.com/document/d/1ptNdtAe4vywW2jldOxbT0iYgRxhA0VjfCAVcYXohEH0/edit

I'd double check with him that this presents his idea correctly, and that he wants it quoted in the PEP (his remarks were not made in the public domain).

All reactions

Sorry, something went wrong.

Uh oh!

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

@Zac-HD Zac-HD changed the title PEP 9999: Enriching Exceptions with Notes PEP 678: Enriching Exceptions with Notes Dec 21, 2021
Zac-HD added 2 commits December 21, 2021 13:16
@Zac-HD
PEP 678: renumber and copyedit
de02a94
@Zac-HD
PEP 678: add content-type
467f41f
@JelleZijlstra

JelleZijlstra commented Dec 21, 2021

Copy link
Copy Markdown
Member

Unless you'd prefer me to wait, I'm planning to merge this PR once CI is green (there's a failure right now). That way, the number is claimed and I can stop pushing the approve CI button. Adding Nathaniel's idea can be done later.

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 commented Dec 21, 2021

Copy link
Copy Markdown
Contributor Author

I did not know about Nathaniel's proposal, but as the doc notes it would not work for Hypothesis; nor in my understanding would it work for tools like friendly-traceback. (though Pytest might be interested!)

I'm happy for this to be merged whenever convenient; I can add a section in "Rejected Ideas" (or "related and deferred ideas", perhaps) once I have a better understanding of the traceback-notes proposal.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Zac-HD Zac-HD mentioned this pull request Dec 21, 2021
bpo-45292: [PEP-654] exception groups and except* documentation python/cpython#30158
Merged
iritkatriel
iritkatriel reviewed Dec 21, 2021
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

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

@Zac-HD

Zac-HD commented Dec 22, 2021

Copy link
Copy Markdown
Contributor Author

@encukou asks in python/cpython#30158 (comment):

Complicated or not, is string hacking the right thing to do? And how much can Hypothesis assume about __note__?

Well, what's the alternative? "String or None" seems much simpler than e.g. a mutable list of sub-notes!

And all Hypothesis, or any other library, needs to assume is that the note is either None (and can be replaced), or some string - and can have another note joined to it. with one or more newlines if desired.

Consider this silly example, which could be refactored in various ways:

    def test_silly(self):
        for a in range(10):
            with self.subTest(a=a):
                for b in range(10):
                    with self.subTest(b=b):
                        self.assertNotEqual(a*b, 12)

Today's unittest has its own way of handling multiple exceptions, but if subTest it was being written today, would __note__ not be a good fit here? If it would, how would subTest be expected to work, and how should it interoperate with Hypothesis?

Yes, __note__ might be a good fit for subTest if it were being designed today.

Hypothesis sees test failures as "an exception raised from a callable", and would concatenate it's note to subTest's.

The result might look something like:
AssertionError: 12 is equal to 12
subTest: a=2
subTest: b=6

Falsifying example: test_silly(
    x=...,  # But what are we generating here, and why not include `a` and `b`?
)

Though note that Hypothesis is generally a replacement for rather than a complement to subTest: instead of looping over some data, I'd have Hypothesis generate examples directly and shrinking takes the place of labelling. That might look like:

@given(a=st.integers(0, 10), b=st.integers(0, 10))
def test_silly(self, a, b):
    self.assertNotEqual(a*b, 12)
AssertionError: 12 is equal to 12
Falsifying example: test_silly(
    a=2,
    b=6,
)
All reactions

Sorry, something went wrong.

Uh oh!

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

@Zac-HD
PEP 678: splitting copies note
908c09c
@iritkatriel

iritkatriel commented Dec 22, 2021

Copy link
Copy Markdown
Member

Is this another use case for note? I wonder why it's written like this instead of chaining. Maybe it predates chaining.

https://github.com/python/cpython/blob/f4c03484da59049eb62a9bf7777b963e2267d187/Lib/urllib/request.py#L1794

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel

iritkatriel commented Dec 22, 2021

Copy link
Copy Markdown
Member

That example also points to an ambiguity in the note-on-traceback suggestion. Does the note belong to the exception or the traceback? In other words - should with_traceback copy over the notes, or create a deep copy of the traceback with the notes cleared?

All reactions

Sorry, something went wrong.

Uh oh!

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

@Zac-HD

Zac-HD commented Dec 22, 2021

Copy link
Copy Markdown
Contributor Author

Is this another use case for note? I wonder why it's written like this instead of chaining. Maybe it predates chaining.

python/cpython@f4c0348/Lib/urllib/request.py#L1794

This goes right back to (at least) the creation of urllib in 2008, when exception chaining was PEP'd but not yet released in Python 3.0. In this particular case I'd be inclined to reach for exception chaining... or, since it's no longer changing the exception type, just take it out of a try/except block entirely.

That example also points to an ambiguity in the note-on-traceback suggestion. Does the note belong to the exception or the traceback? In other words - should with_traceback copy over the notes, or create a deep copy of the traceback with the notes cleared?

As I understand it, the proposal was to attach notes to particular traceback frames, not the exception. This would also mean that Trio's and Hypothesis' traceback-trimming logic would discard any notes that had (somehow!) been attached to frames we pop off the traceback. @njsmith might elaborate?

All reactions

Sorry, something went wrong.

Uh oh!

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

@JelleZijlstra

JelleZijlstra commented Dec 22, 2021

Copy link
Copy Markdown
Member

FYI CI is failing.

All reactions

Sorry, something went wrong.

Uh oh!

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

iritkatriel
iritkatriel reviewed Dec 22, 2021
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

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

@iritkatriel
fix backpacks
72c4c2b
JelleZijlstra
JelleZijlstra reviewed Dec 22, 2021
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

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

@iritkatriel @JelleZijlstra
remove :meth:
324959f
Co-authored-by: Jelle Zijlstra <[email protected]>
JelleZijlstra
JelleZijlstra approved these changes Dec 22, 2021
View reviewed changes
JelleZijlstra
JelleZijlstra reviewed Dec 22, 2021
View reviewed changes
Comment thread pep-0678.rst Resolved

Uh oh!

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

@JelleZijlstra
Update pep-0678.rst
4c8b1df
@JelleZijlstra
JelleZijlstra merged commit 8c43285 into python:main Dec 22, 2021
@Zac-HD
Zac-HD deleted the exception-note branch December 22, 2021 19:25
@warsaw

warsaw commented Dec 24, 2021 via email

Copy link
Copy Markdown
Member
On Dec 21, 2021, at 23:46, Zac Hatfield-Dodds ***@***.***> wrote: @encukou asks in python/cpython#30158 (comment): Complicated or not, is string hacking the right thing to do? And how much can Hypothesis assume about __note__? Well, what's the alternative? "String or None" seems much simpler than e.g. a mutable list of sub-notes!
I see this PR has been merged, so happy to take this discussion elsewhere (Discuss or python-dev). Did you think about setting __note__ to an object that has a __str__()? I also question whether string hacking is the right thing for this, given that coordination between except clauses within different frameworks might make that exceedingly difficult and fragile. If you could set __note__ to something more structured, then that object could provide a useful API and a __str__() for printing at trackback time. I’m unsure whether this object should be defined in the stdlib or whether it should be left up to __note__ users. Maybe leave room for experimentation for now and see if consensus builds for an API.
All reactions

Sorry, something went wrong.

Uh oh!

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

@Zac-HD

Zac-HD commented Dec 24, 2021

Copy link
Copy Markdown
Contributor Author

Did you think about setting __note__ to an object that has a __str__()? I also question whether string hacking is the right thing for this, given that coordination between except clauses within different frameworks might make that exceedingly difficult and fragile. If you could set __note__ to something more structured, then that object could provide a useful API and a __str__() for printing at trackback time.

I did consider this briefly, but didn't think the extra complexity was justified - I can't think of a scenario where frameworks would want to do anything but either concatenate or replace notes, and that wouldn't require any coordination with strings.

What kind of string hacking are you thinking of which would be fragile?

All reactions

Sorry, something went wrong.

Uh oh!

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

@warsaw

warsaw commented Dec 26, 2021

Copy link
Copy Markdown
Member

What kind of string hacking are you thinking of which would be fragile?

I don't have any specific use cases in mind, so it's just an intuition. Let's say I needed to catch an exception, roll back a transaction, and re-raise the exception. E.g. as in this code. What if I wanted to add a __note__ here saying "The transaction has been aborted". Where in the __note__ would I put that? Sure, I could just concatenate this, but maybe I wanted to be more clever and try to put this somewhere else in the exception note. Now I have to parse the string and figure out where to put it, but all that parsing will be pretty fragile. E.g. maybe there's some library layered in there that's putting in its own __note__ and that text changes between versions of the library. The mental model I have is something like doctests where you're printing out exceptions. You need to be very clever about that or your tests are closely tied to the exact language of the exception, which has known history of being easily broken.

As I said, I don't have a specific example in mind, so maybe none of this will be a problem in practice. I don't even have a better API in mind. I'm also not sure we can design the right API without some time with this feature released out in the wild. I'm just trying to raise some concerns based on previous, somewhat similar experiences. I think it might be sufficient enough to allow None, a str or an object with a __str__() and allow experimentation to guide us.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Zac-HD Zac-HD mentioned this pull request Dec 27, 2021
PEP 678: more rejected ideas #2205
Merged
@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

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@iritkatriel iritkatriel iritkatriel left review comments
@gvanrossum gvanrossum gvanrossum left review comments

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 @gvanrossum @brettcannon @JelleZijlstra @warsaw @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.