Skip to content

bpo-42179: Clarify exception chaining #23160

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

Merged
merged 5 commits into from
Dec 16, 2020

Conversation

greatvovan
Copy link
Contributor

@greatvovan greatvovan commented Nov 5, 2020

Add an important detail and provide a link.

https://bugs.python.org/issue42179

Clarify exception chaining behaviour and give a reference to the library documentation.
@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Nov 5, 2020
@greatvovan greatvovan changed the title bpo 42179: Clarify exception chaining bpo-42179: Clarify exception chaining Nov 5, 2020
@merwok
Copy link
Member

merwok commented Nov 5, 2020

I agree with @methane that this degree of detail is too much for the tutorial.

@greatvovan
Copy link
Contributor Author

@merwok can you post (and elaborate) your opinion in the BPO ticket? I think it's better to keep discussion on the change in one place. I can reduce degree of detail, too.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Thanks @greatvovan for the PR. Let's update this after #23162 lands. I like the sentence that I've flagged and think it would be helpful.

inside an exception handler or :keyword:`finally` section. Exception chaining
can be disabled by using ``from None`` idiom:
inside an exception handler or :keyword:`finally` section, however in this case
``__context__`` attribute of the exception is used. Chaining with
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend submitting this sentence right before the builtin exceptions sentence after #23162 lands. Then add "Chaining with ... takes priority."

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

@greatvovan
Copy link
Contributor Author

I have made the requested changes; please review again.

@willingc the sentence you suggested could not be used as is in the new variant of text as the context is bit lost after removing mentions of special attributes. I added a new abstract in the end of the section, please let me know what you think.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@willingc: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from willingc November 6, 2020 20:19
@@ -281,17 +281,17 @@ chaining exceptions. For example::
This can be useful when you are transforming exceptions. For example::

>>> def func():
... raise IOError
... raise ConnectionError
Copy link
Member

Choose a reason for hiding this comment

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

Nice! One OSError was missed on line 294

Remove mentioning of special attributes as folks think it's too much for beginners.
@greatvovan
Copy link
Contributor Author

greatvovan commented Nov 9, 2020

Please review the final version.

I removed my proposal paragraph about __cause__ and __context__ as nobody seems to be happy with it. I kept only changes about exception type (https://bugs.python.org/issue42179#msg380435).

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Thanks @greatvovan and good catch on ConnectionError. @merwok If you are good with this as it stands, please merge. Thanks ☀️

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Dec 16, 2020
@merwok
Copy link
Member

merwok commented Dec 16, 2020

Thanks for merging Carol!

FYI the commit message is messy due to github’s poor UI.

@willingc
Copy link
Contributor

Sorry about the commit message. Thanks @merwok.

@greatvovan greatvovan deleted the bugfix/bpo-42179 branch December 16, 2020 17:10
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
* Update errors.rst

Clarify exception chaining behaviour and give a reference to the library documentation.

* Update errors.rst

Wording

* Update errors.rst

Spelling

* Update errors.rst

Remove mentioning of special attributes as folks think it's too much for beginners.
@hugovk hugovk mentioned this pull request Mar 6, 2022
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 stale Stale PR or inactive for long period of time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants