Skip to content

bpo-40899: Document exception raised when module cannot be imported#27709

Closed
dukecat0 wants to merge 6 commits intopython:mainfrom
dukecat0:patch-2
Closed

bpo-40899: Document exception raised when module cannot be imported#27709
dukecat0 wants to merge 6 commits intopython:mainfrom
dukecat0:patch-2

Conversation

@dukecat0
Copy link
Copy Markdown
Contributor

@dukecat0 dukecat0 commented Aug 10, 2021 •
edited by bedevere-bot
Loading

@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir labels Aug 10, 2021
ambv
ambv requested changes Aug 10, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@ambv ambv left a comment

Choose a reason for hiding this comment

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

  1. Note Ronald's comment on arbitrary exceptions being raised from a module body while it's being imported.
  2. Please don't insert error handling information in the leading paragraph. Look at other functions in functions.rst, they have exception information listed further in the text, usually as the last paragraph describing them.

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Aug 10, 2021

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.

@dukecat0 dukecat0 marked this pull request as draft August 12, 2021 13:11
@gpshead
Copy link
Copy Markdown
Member

gpshead commented Aug 18, 2021

  1. Note Ronald's comment on arbitrary exceptions being raised from a module body while it's being imported.
  2. Please don't insert error handling information in the leading paragraph. Look at other functions in functions.rst, they have exception information listed further in the text, usually as the last paragraph describing them.

while I've Approved this change from the standpoint of not being a bad thing to document, I do think the above should be addressed first.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 18, 2021

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Sep 18, 2021
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.11 only security fixes label May 20, 2022
@j13r j13r mannequin mentioned this pull request Jul 7, 2022
Closed
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Aug 11, 2022
@iritkatriel
Copy link
Copy Markdown
Member

iritkatriel commented Nov 23, 2022

@meowmeowmeowcat This has been awaiting changes for over a year. Are you planning to finish this or shall we close it so someone else can pick it up?

@iritkatriel iritkatriel added the pending The issue will be closed if no feedback is provided label Nov 23, 2022
iritkatriel
iritkatriel requested changes Nov 23, 2022
View reviewed changes
Copy link
Copy Markdown
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.

See #27709 (comment).

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Nov 23, 2022

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.

@dukecat0 dukecat0 marked this pull request as ready for review November 24, 2022 09:27
@dukecat0
Copy link
Copy Markdown
Contributor Author

dukecat0 commented Nov 24, 2022

Are you planning to finish this or shall we close it so someone else can pick it up?

I am going to finish this. Sorry for the late response!

I have made the requested changes; please review again

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Nov 24, 2022

Thanks for making the requested changes!

@iritkatriel, @ambv, @gpshead: please review the changes made to this pull request.

dukecat0
dukecat0 commented Nov 24, 2022
View reviewed changes
Comment thread Doc/library/importlib.rst Outdated
@iritkatriel iritkatriel removed the pending The issue will be closed if no feedback is provided label Nov 24, 2022
iritkatriel
iritkatriel reviewed Nov 24, 2022
View reviewed changes
Comment thread Doc/library/functions.rst Outdated
Comment thread Doc/library/importlib.rst Outdated
@dukecat0 @iritkatriel
Fix typo
c494ed6 
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
iritkatriel
iritkatriel reviewed Nov 24, 2022
View reviewed changes
Comment thread Doc/library/functions.rst
Comment on lines +2021 to +2022
An :exc:`ImportError` exception can be raised if the module cannot be imported
successfully.
Copy link
Copy Markdown
Member

@iritkatriel iritkatriel Nov 24, 2022

Choose a reason for hiding this comment

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

I'm not sure I like this wording - what do you mean "can be raised", it sounds like it is possible for import to fail and not raise an exception.

Copy link
Copy Markdown
Contributor Author

@dukecat0 dukecat0 Nov 24, 2022

Choose a reason for hiding this comment

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

Note Ronald's comment on arbitrary exceptions being raised from a module body while it's being imported.

It means arbitrary exceptions can also be raised.

Copy link
Copy Markdown
Member

@iritkatriel iritkatriel Nov 24, 2022

Choose a reason for hiding this comment

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

I understand the intent, but I don't think that's the only possible interpretation of the current wording.

Copy link
Copy Markdown
Contributor Author

@dukecat0 dukecat0 Nov 24, 2022

Choose a reason for hiding this comment

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

I just found out that #94662 is also doing the same thing. Would you prefer that one?

@hugovk hugovk removed the needs backport to 3.10 only security fixes label Apr 7, 2023
@zormit zormit mentioned this pull request Jul 23, 2023
Merged
@dukecat0 dukecat0 closed this Aug 13, 2023
@dukecat0 dukecat0 deleted the patch-2 branch August 13, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel requested changes

@ambv ambv Awaiting requested review from ambv

@gpshead gpshead Awaiting requested review from gpshead

Assignees

No one assigned

Labels

awaiting change review docs Documentation in the Doc dir needs backport to 3.11 only security fixes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@dukecat0 @bedevere-bot @gpshead @iritkatriel @ambv @hugovk @serhiy-storchaka @the-knights-who-say-ni @ezio-melotti