gh-127598: Improve ModuleNotFoundError when -S is passed#136821
gh-127598: Improve ModuleNotFoundError when -S is passed#136821encukou merged 8 commits intopython:mainfrom
Conversation
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
Thanks, please add a test case and news entry.
ilovelinux
left a comment
ilovelinux
left a comment
There was a problem hiding this comment.
Should we write a test for that?
| elif exc_type and issubclass(exc_type, ModuleNotFoundError) and \ | ||
| sys.flags.no_site: | ||
| self._str += ". Site initialization is disabled, did you forget to add the \ | ||
| site-package directory to sys.path?" |
There was a problem hiding this comment.
Traceback message has too many spaces in the middle:
ModuleNotFoundError: No module named 'foo'. Site initialization is disabled, did you forget to add the site-package directory to sys.path?
There was a problem hiding this comment.
It should be fixed now, please test it again
|
@ZeroIntensity I've also add an additional check to exception as a suggestion from @encukou, I'll add also a test for it |
|
@ZeroIntensity I have pushed your feedbacks and I also add a test for it, let me know if something else is necessary. |
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
Mostly LGTM, with two minor comments.
| cmd = [sys.executable, '-S', '-c', 'import boo'] | ||
| result = subprocess.run( | ||
| cmd, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| text=True | ||
| ) |
There was a problem hiding this comment.
We have a helper for doing this: test.support.assert_python_ok. Would you mind switching to that?
There was a problem hiding this comment.
ok I've changed the to use it, although the version assert_python_failure as we looking for an exception
| @@ -0,0 +1,2 @@ | |||
| Improve :exc:`ModuleNotFoundError` by adding flavour text to exception when the | |||
There was a problem hiding this comment.
Oops, missed this the first time:
| Improve :exc:`ModuleNotFoundError` by adding flavour text to exception when the | |
| Improve :exc:`ModuleNotFoundError` by adding flavour text to the exception when the |
| self._str += f". Did you mean: '{suggestion}'?" | ||
| elif exc_type and issubclass(exc_type, ModuleNotFoundError) and \ | ||
| sys.flags.no_site and \ | ||
| getattr(exc_value, "name", None) not in sys.builtin_module_names: |
There was a problem hiding this comment.
This feature should use stdlib_module_names, not builtin module names.
| getattr(exc_value, "name", None) not in sys.builtin_module_names: | |
| getattr(exc_value, "name", None) not in sys.stdlib_module_names: |
(It could be tested by importing a non-existent stdlib module, like msvcrt on *nix or grp on Windows. The note should not be added for those -- you can't solve a "wrong platform" issue by adding the site-packages directory.)
There was a problem hiding this comment.
I implemented the feedback, and it case stdlib the message it's not thrown :)
| ) | ||
|
|
||
| code = """ | ||
| import msvcrt |
There was a problem hiding this comment.
This test will fail on Windows, where msvcrt is available.
Could you either
- go back to testing
booafter adding it tostdlib_module_names, or - use
grpon Windows, andmsvcrteverywhere else?
encukou
left a comment
encukou
left a comment
There was a problem hiding this comment.
This version looks great! Thank you for the fix!
4 participants
This (partially) solves gh-127598 by adding flavour text to exception when the argument '-S' is passed.
ImportErrorfor common issues #127598