gh-85076: Document exceptions that can be raised by importlib.import_module #94662
Conversation
|
@FFY00 I didn't because it's just a documentation change. |
Doc/library/importlib.rst
Outdated
| If the module cannot be imported, :func:`import_module` will raise an | ||
| :exc:`ImportError`. If the module is found and loaded, but the code in the | ||
| module raises an exception, :func:`import_module` will pass that exception | ||
| to the module that called it. |
There was a problem hiding this comment.
| If the module cannot be imported, :func:`import_module` will raise an | |
| :exc:`ImportError`. If the module is found and loaded, but the code in the | |
| module raises an exception, :func:`import_module` will pass that exception | |
| to the module that called it. | |
| If the module cannot be found or loaded, :func:`import_module` will raise an | |
| :exc:`ImportError`. |
"cannot be imported" is very generic and could conceivably apply to explicitly raised exceptions, like in the second half of this paragraph, so makes sense to be more specific.
I think we could just omit the second half of the paragraph. Talking about "passing exceptions" makes it sound more complicated than it is. The behaviour here is intuitive and already documented by the sentence "This means all semantics of the function are derived from :func:importlib.__import__."
There was a problem hiding this comment.
Doesn't "cannot be found or loaded" imply that that's the behavior if the module cannot be loaded because code in the module raised an exception? Maybe "If the module cannot be found or its files cannot be accessed..." instead? That makes it clear that ImportError is raised in cases like the module not being found or being inaccessible due to file permissions, not cases where it is found but not successfully loaded.
There was a problem hiding this comment.
I suggest to look further down, where I tried to consolidate things. (sorry for the spam, I hope it's helpful for readers 😅)
Doc/library/importlib.rst
Outdated
| If the module cannot be imported, :func:`import_module` will raise an | ||
| :exc:`ImportError`. If the module is found and loaded, but the code in the | ||
| module raises an exception, :func:`import_module` will pass that exception | ||
| to the module that called it. |
There was a problem hiding this comment.
Trying to incorporate everything discussed at the same time.
@brettcannon's suggestion:
- is more specific on the type of exception that can be expected.
- does not incorporate the remark from @hauntsaninja, that criticises the fact that "cannot be imported" is very generic.
In the first thread @kj7rrv suggested a nice way of explaining how a found module can't be imported. Not sure if it's now too specific, but let's continue the discussion:
| If the module cannot be imported, :func:`import_module` will raise an | |
| :exc:`ImportError`. If the module is found and loaded, but the code in the | |
| module raises an exception, :func:`import_module` will pass that exception | |
| to the module that called it. | |
| If the module cannot be found or its files cannot be accessed, | |
| :func:`import_module` will raise :exc:`ImportError` or an appropriate | |
| subclass like :exc:`ModuleNotFoundError`. |
|
Hi @kj7rrv, are you still interested in getting this merged? I found this by looking at P.S:. If you're busy / have other priorities, I can also open another PR and we can close this one. But given there is already a 2nd open PR, I didn't want to add a 3rd one 😅 |
Co-authored-by: Brett Cannon <[email protected]>
|
@zormit Hi! I applied your suggested change. I am still interested in having it merged; is there anything else that needs to be changed? |
|
I'm not sure why it bedevere still says this is @kj7rrv Also a clarifying question: Did you mean to apply my changes? I think you have accepted the changes by Brett instead of mine. They are a bit less detailed. It would be okay for me, I could approve it, given it's much better than nothing, but I would be curious about your reasoning behind it. |
It's because @kj7rrv didn't say the triggering phrase as specified in #94662 (comment) to refresh review requests. |
|
@kj7rrv could you add a comment that includes this line? |
|
I have made the requested changes; please review again.
|
|
Thanks for making the requested changes! @brettcannon, @FFY00: please review the changes made to this pull request. |
Closes #85076