Skip to content

Conversation

@yangdanny97
Copy link
Contributor

@yangdanny97 yangdanny97 commented Jun 14, 2024

@ghost
Copy link

ghost commented Jun 14, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app bedevere-app bot added docs Documentation in the Doc dir skip news awaiting review labels Jun 14, 2024
@JelleZijlstra JelleZijlstra self-requested a review June 14, 2024 21:01

An :keyword:`!except*` clause must have a matching type,
and this type cannot be a subclass of :exc:`BaseExceptionGroup`.
An :keyword:`!except*` clause must have a matching type or a tuple of matching
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth expanding this further:

  • The docs for except (the previous section) never say that each type must be a subtype of BaseException. That's probably worth adding.
  • One important difference between except and except* is that the former allows "bare" except clauses (just except:), but except* doesn't. I think that's worth mentioning in a sentence by itself.
  • We don't say what happens if the type is a subclass of BaseExceptionGroup.

So I'd suggest something like this:

Suggested change
An :keyword:`!except*` clause must have a matching type or a tuple of matching
Unlike :keyword:`except` clauses, :keyword:`!except*` clauses must have a matching
expression; `except*:` is not valid syntax. When the handler is evaluated, the
expression must evaluate to a type or a tuple containing types. Each type must be a
subclass of :exc:`BaseException` and must not be a subclass of :exc:`BaseExceptionGroup`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've added a mention that the runtime will raise a TypeError if the type is wrong, and updated the except section to have similar language.

Should the latter be split into a separate PR, or should I just update the title/summary of this PR?

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.

This looks right, cc @iritkatriel for except*.

yangdanny97 and others added 2 commits June 15, 2024 06:55
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
yangdanny97 and others added 2 commits June 15, 2024 09:04
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Copy link

@offensive-vk offensive-vk left a comment

Choose a reason for hiding this comment

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

Nice explanation!

@iritkatriel
Copy link
Member

I think this PR should just resolve the issue it is associated with. As it is now it is doing a lot more than that.
For example, the fact that an incorrect exception expression results in TypeError at runtime is an implementation detail, it is not currently (as far as I know) documented in the language spec.

Can you limit the PR to just resolve the problem raised in the issue?

@yangdanny97
Copy link
Contributor Author

yangdanny97 commented Jun 15, 2024

I think this PR should just resolve the issue it is associated with. As it is now it is doing a lot more than that. For example, the fact that an incorrect exception expression results in TypeError at runtime is an implementation detail, it is not currently (as far as I know) documented in the language spec.

Can you limit the PR to just resolve the problem raised in the issue?

I'll remove the TypeError part, though I think the rest of it is kind of difficult to untangle because the except* docs now references the except docs (with the "in addition to"/"unlike" wording), so it might make sense to keep the changes together.

Do you have a preference for how to split it up?

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.

I think it became a bit wordy. I tried to reword it more concisely.

matching expression; ``except*:`` is not valid syntax. In addition to the
requirements for matching expressions in :keyword:`except` clauses, the
types in the matching expression of an :keyword:`!except*` clause cannot
be :exc:`BaseExceptionGroup` or a subclass of :exc:`BaseExceptionGroup`.
Copy link
Member

Choose a reason for hiding this comment

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

(Above we talk about "exception group" when we refer to BaseException and its subclasses).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm confused about this comment - is it addressed in your suggestions already?

The other suggestions on this PR which I've merged in seem to have removed all references to BaseException and BaseExceptionGroup classnames in this doc.

Feels like explicitly mentioning the class names would be more clear, wouldn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this comment is addressed.

Since we talk about "exception group" in the previous paragraphs, assuming the reader knows what we mean there, I don't think specifying the type here would add clarity.

yangdanny97 and others added 3 commits June 17, 2024 10:58
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
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, I'll let @JelleZijlstra have another look.

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.

Looks good, just a wording suggestion

For an :keyword:`!except` clause with an expression, the
expression must evaluate to an exception type or a tuple of exception types.
The raised exception matches an :keyword:`!except` clause whose expression evaluates
to the class or a :term:`non-virtual base class <abstract base class>` of the exception object,
Copy link
Member

Choose a reason for hiding this comment

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

This is a pre-existing problem and doesn't have to be solved in this PR, but I don't think linking to the definition of "abstract base class" here is great.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@JelleZijlstra JelleZijlstra enabled auto-merge (squash) June 19, 2024 18:43
@JelleZijlstra JelleZijlstra added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jun 19, 2024
@JelleZijlstra JelleZijlstra merged commit 58b3f11 into python:main Jun 19, 2024
@miss-islington-app
Copy link

Thanks @yangdanny97 for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 19, 2024
…nGH-120523)

(cherry picked from commit 58b3f11)

Co-authored-by: Danny Yang <yangdanny97@users.noreply.github.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented Jun 19, 2024

GH-120750 is a backport of this pull request to the 3.13 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 19, 2024
…nGH-120523)

(cherry picked from commit 58b3f11)

Co-authored-by: Danny Yang <yangdanny97@users.noreply.github.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 19, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jun 19, 2024

GH-120751 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Jun 19, 2024
JelleZijlstra added a commit that referenced this pull request Jun 19, 2024
…20523) (#120751)

(cherry picked from commit 58b3f11)

Co-authored-by: Danny Yang <yangdanny97@users.noreply.github.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
JelleZijlstra added a commit that referenced this pull request Jun 19, 2024
…20523) (#120750)

(cherry picked from commit 58b3f11)

Co-authored-by: Danny Yang <yangdanny97@users.noreply.github.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
…n#120523)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
…n#120523)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
…n#120523)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation in the Doc dir skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify documentation for except*

4 participants