-
Notifications
You must be signed in to change notification settings - Fork 250
177 support from import sinks #198
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
177 support from import sinks #198
Conversation
* Allow trigger words to be fully qualified to reduce false positives
* This will give fully qualified names for blackboxes like flask * Improve readability by using keyword arguments
Please review this change carefully; It changes the contents of import_alias_mapping and I'm not 100% this has no unintended side-effects.
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 great to me, thank you so much for making this 👍 Very high-quality code
I left a few optional feedback comments, but the functionality looks great
edit: Don't worry about Codeclimate, it complains about everything
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.
Thank you for doing this 🙏 :) kwargs always improve readability
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.
You're definitely improving the readability of this code, thank you :) The less indentation the better
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 makes the output more readable as well, awesome job 🎉
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.
The main thing I'm unsure about is, why I was passing None for real_names, but it was probably wrong :)
I do remember I wrote a lot of tests in the import PRs, so I'm somewhat confident this code is well covered. (Granted it was when I was an even worse engineer than I am now 😁 ).
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.
Great test 🎈 🥇
Uh oh!
There was an error while loading. Please reload this page.
Remember import aliases for built-in and black box functions.
os.systemand it will match even if the import us done usingfrom os import systemThis is an incompatible change. The labels for function calls changed (they are now fully qualified, regardless of how the function was imported).
This solves #177