- Notifications
You must be signed in to change notification settings - Fork673
chore: reduce usage of 'from mod_foo import class_foo'#2872
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
6a1be5d
tobcebcf7
CompareThere 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@JohnVillalovos I guess it makes sense to align as we have a huge mix now in the library.
But I think this PR doesn't really replace class/function imports, it replaces from-imports with plain imports. Which I think is not the idea behind Google's styleguide that I think this is trying to follow. Seehttps://google.github.io/styleguide/pyguide.html#22-imports. I think their idea would befrom requests import auth
->auth.AuthBase
.
I'm not sure the full path is always needed when referencing especially with modern IDEs and code intelligence enabled on GitHub/GitLab?
d0e54a1
toa2302d7
CompareReduce usage of doing: from SOME_MODULE import SOME_CLASS_OR_FUNCTIONInstead use: import SOME_MODULEAnd then use the full module name. This improves readability and makesobvious to the reader where the class or function has come from.
a2302d7
tof251642
CompareJohnVillalovos commentedJun 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.
@nejch Thanks for the review.
It replaces some class/function imports. Like And the other ones I personally like better seeing the full path.
Well as someone who is rocking So I'm not sure how you want to proceed on this. Are you opposed to all of the changes? Some of the changes? Something else? And I'm fine closing this PR. Not a big deal to not get it merged 😊 Thanks! |
nejch commentedJun 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.
Thanks@JohnVillalovos I'd maybe go with a compromise and follow Google's styleguide then, maybe like their example in thedecision chapter:
Otherwise for example if we wanted to be consistent and add this to our tests as well, we'd have to go with things like Sorry, I'm not too familiar with vim specifics, I was under the impression the language server protocol plugins and neovim would also enable this 🙇 Edit: also I'd try to make sure that those are always done in dedicated PRs like this (maybe we can also add those style changes to rev ignore), so we don't mix them with functional changes. |
Be aware that |
Realistically the time difference is insignificant in this code between the two. In other Python projects maybe, but with the network operations that happen it drowns out any microsecond optimizations. |
This Pull Request (PR) was marked stale because it has been open 90 days with no activity. Please remove the stale label or comment on this PR. Otherwise, it will be closed in 15 days. |
Uh oh!
There was an error while loading.Please reload this page.
Reduce usage of doing:
from SOME_MODULE import SOME_CLASS_OR_FUNCTION
Instead use:
import SOME_MODULE
And then use the full module name. This improves readability and makes obvious to the reader where the class or function has come from.