Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-95754: Better error when script shadows a standard library or third party module#113769
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
hauntsaninja commented Jan 6, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
9279982 to 6f14b0dComparef69e0b5 to d4111e5Comparebedevere-bot commented Jan 6, 2024
🤖 New build scheduled with the buildbot fleet by @hauntsaninja for commit d4111e5 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
d4111e5 to 0933591Comparehauntsaninja commented Jan 6, 2024
Oh actually looks like I can use stdlib_module_names.h do get sys.stdlib_module_names in C. And I can of course do the path munging in C as well. |
4c09d8e to 9cb7291Compare9cb7291 to 5e22968Comparehauntsaninja commented Jan 7, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Should I disable this if Oh hmm, (edit from later: I discovered |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ericsnowcurrently left a comment
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.
A couple more minor comments.
ericsnowcurrently commented Mar 8, 2024
FYI, I think we're at the tail end of my review and I don't expect to have much more feedback to offer. Thanks for working on this. You'll still want to double-check with @pablogsal and anyone else with a vested interest. |
hauntsaninja commented Mar 8, 2024
Thank you so much for the detailed review! |
hauntsaninja commented Mar 31, 2024
Does anyone have any more comments? If not, I'd love a green check mark to unblock the merge. (And of course I'll be happy to address any post-merge follow-ups) |
ericsnowcurrently commented Apr 1, 2024
We need to be sure the other reviewers are on board, especially @pablogsal. |
ericsnowcurrently commented Apr 1, 2024
AlexWaygood left a comment
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 previously gave feedback on the error message, which looks great to me now! Not confident enough to give other kinds of feedback on this sort of PR 😄
JelleZijlstra left a comment
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.
No need to wait for me before merging, as other reviewers know a lot more about the import system. However, a few minor points.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
hauntsaninja commented Apr 20, 2024
This has been quiet for a while and I'd love to get it in ahead of beta1, so I will probably go ahead and merge soon. Please let me know if you have additional feedback and I'll follow up in additional PRs. Thanks to everyone who left reviews, in particular to ericsnowcurrently! |
erlend-aasland left a comment
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.
Looks good to me. I added some minor suggestions (C idioms and a PEP 7 nit); feel free to ignore them.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
hauntsaninja commented Apr 23, 2024
Thanks Erlend! |
henryiii commented Jun 29, 2024
This is great, but I'm greedy now: what about the other form? This works for AttributeError $ touch pathlib.py $ python3.13 -c "import pathlib; pathlib.Path"Traceback (most recent call last): File "<string>", line 1, in <module> import pathlib; pathlib.Path ^^^^^^^^^^^^AttributeError: module 'pathlib' has no attribute 'Path' (consider renaming '/Users/henryschreiner/git/presentations/python313/pathlib.py' since it has the same name as the standard library module named 'pathlib' and the import system gives it precedence) $ python3.13 -c "from pathlib import Path"Traceback (most recent call last): File "<string>", line 1, in <module> from pathlib import PathImportError: cannot import name 'Path' from 'pathlib' (/Users/henryschreiner/git/presentations/python313/pathlib.py) |
This is a very common mistake for beginners. This PR tries to detect the most common case: where a file in the current directory ends up on sys.path and shadows a standard library module. (My previous PR #112577 was only marginally helpful and wouldn't help with many such errors)
The first commit in this PR is just refactoring.
📚 Documentation preview 📚: https://cpython-previews--113769.org.readthedocs.build/