-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-127598: Improve ModuleNotFoundError when -S is passed #136821
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
gh-127598: Improve ModuleNotFoundError when -S is passed #136821
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, please add a test case and news entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we write a test for that?
Lib/traceback.py
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Misc/NEWS.d/next/Core_and_Builtins/2025-07-19-17-08-09.gh-issue-127598.Mx8S-y.rst
Outdated
Show resolved
Hide resolved
|
@ZeroIntensity I have pushed your feedbacks and I also add a test for it, let me know if something else is necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, with two minor comments.
Lib/test/test_traceback.py
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
Lib/traceback.py
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented the feedback, and it case stdlib the message it's not thrown :)
Lib/test/test_traceback.py
Outdated
| ) | ||
|
|
||
| code = """ | ||
| import msvcrt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version looks great! Thank you for the fix!
This (partially) solves gh-127598 by adding flavour text to exception when the argument '-S' is passed.
ImportErrorfor common issues #127598