- Notifications
You must be signed in to change notification settings - Fork675
chore: remove usage of 'from ... import *'#1319
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 ourterms of service andprivacy 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
codecov-io commentedFeb 23, 2021
Codecov Report
@@ Coverage Diff @@## master #1319 +/- ##==========================================- Coverage 80.72% 79.95% -0.78%========================================== Files 69 71 +2 Lines 3627 3747 +120 ==========================================+ Hits 2928 2996 +68- Misses 699 751 +52
Flags with carried forward coverage won't be shown.Click here to find out more.
Continue to review full report at Codecov.
|
JohnVillalovos commentedFeb 23, 2021 • 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.
I updated the files using this script and than ran |
nejch 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.
Thanks for this@JohnVillalovos! I'm happy to see more star imports gone 😁
I'm just thinking that since these will be used in every new module as resources are added, perhaps we could import them all as needed into the module's namespace to keep a smaller footprint in the class definitions (and this diff would also be smaller). Especially since some of them will also grow with type annotations. Like this:
https://github.com/encode/httpx/blob/0f280af8b170ed5cc48c12a894f71a8b5762f748/httpx/_client.py#L34-L47 and
https://github.com/encode/httpx/blob/0f280af8b170ed5cc48c12a894f71a8b5762f748/httpx/__init__.py#L6-L35
And similar in requests, although I prefer the above formatting in httpx (black probably won't let us do otherwise anyway 😄 ):
I think this would be a bit more readable for future contributors while still explicit. WDYT? Since you were already able to script this it might be a quick change for you :) The only downside would be adding mixins to imports as they get added to modules each time.
JohnVillalovos commentedFeb 23, 2021
Sure. I think I could modify the script to do that... Let me look into it when I finish my work day. Thanks for the review! |
In gitlab/v4/objects/*.py remove usage of: * from gitlab.base import * * from gitlab.mixins import *Change them to: * from gitlab.base import CLASS_NAME * from gitlab.mixins import CLASS_NAMEProgrammatically update code to explicitly import needed classes only.After the change the output of: $ flake8 gitlab/v4/objects/*py | grep 'REST\|Mixin'Is empty. Before many messages about unable to determine if it was avalid name.
JohnVillalovos commentedFeb 25, 2021
Sweet. Less 'import *' in the code 😊 Thanks for the merge! |
Uh oh!
There was an error while loading.Please reload this page.
In gitlab/v4/objects/*.py remove usage of:
from gitlab.base import *
from gitlab.mixins import *
Change them to:
Programmatically update code to explicitly import needed classes only.
After the change the output of:
$ flake8 gitlab/v4/objects/*py | grep 'REST|Mixin'
Is empty. Before many messages about unable to determine if it was a
valid name.