-
Notifications
You must be signed in to change notification settings - Fork 461
Implements Python error hint when importing from src.
#5407
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
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #5407 will not alter performanceComparing Summary
Footnotes
|
444565f to
206f59a
Compare
| exc.add_note( | ||
| "If your main module is inside the 'src' directory then your import " + | ||
| "statement shouldn't include a 'src.' prefix") | ||
| raise exc |
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.
To avoid memory leaks you should do:
try:
raise exc
finally:
del exc
Or ideally move the whole thing into a Python try/catch.
It's important to remember that exceptions hold references to frame objects so keeping them alive also keeps all the local variables from all those scopes alive.
|
What about if the import happens in the fetch handler? |
| pyodide.runPython(` | ||
| import sys | ||
| exc = sys.last_value | ||
| if exc.name == "src": |
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 will raise an attribute error if exc doesn't have a name attribute. You need to check if it's a ModuleNotFoundError.
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.
If the test suite is currently passing let's make sure there's a test where some other error is raised at top level and make sure it doesn't get messed up.
Test is in EW because wd-test doesn't make it easy to assert on script startup failures afaik.
Test Plan