-
Notifications
You must be signed in to change notification settings - Fork 158
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
Fix user import when it overlaps with runtime library modules #1743
Conversation
There could be other approaches to fix #1737, for example, changing/reversing the priority of This PR fixes the issue by pruning the paths to be searched (as we need not re-search for the package/module). |
This is ready. Please review and share feedback. |
LGTM! Thanks for fixing this! |
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 think it's fine. @czgdp1807 can you also please review?
@@ -0,0 +1,2 @@ | |||
def hi_from_user_sys(): | |||
print("hi from user sys!") |
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.
Probably return an integer value from here.
integration_tests/test_import_05.py
Outdated
@@ -0,0 +1,3 @@ | |||
from test_import.sys import hi_from_user_sys | |||
|
|||
hi_from_user_sys() |
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.
And then assert here. So that we know that we are not having any hidden bugs.
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.
Done! Thanks for the guidance!
I am adding this to auto-merge as it seems approved. |
fixes #1737.