Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
JohnVillalovos wants to merge1 commit intomain
base:main
Choose a base branch
Loading
fromjlvillal/reduce_from_imports

Conversation

JohnVillalovos
Copy link
Member

@JohnVillalovosJohnVillalovos commentedMay 17, 2024
edited
Loading

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.

@JohnVillalovosJohnVillalovosforce-pushed thejlvillal/reduce_from_imports branch from6a1be5d tobcebcf7CompareMay 17, 2024 00:09
@JohnVillalovosJohnVillalovos changed the titlechore reduce usage of 'from mod_foo import class_foo'chore: reduce usage of 'from mod_foo import class_foo'May 17, 2024
Copy link
Member

@nejchnejch left a 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?

@JohnVillalovosJohnVillalovosforce-pushed thejlvillal/reduce_from_imports branch 2 times, most recently fromd0e54a1 toa2302d7CompareMay 17, 2024 14:50
Reduce 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.
@JohnVillalovosJohnVillalovosforce-pushed thejlvillal/reduce_from_imports branch froma2302d7 tof251642CompareJune 5, 2024 16:14
@JohnVillalovos
Copy link
MemberAuthor

JohnVillalovos commentedJun 6, 2024
edited
Loading

@nejch Thanks for the review.

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.

It replaces some class/function imports. Likerequests.CaseInsensitiveDict,enum.Enum,enum.IntEnum, andpathlib.Path

And the other ones I personally like better seeing the full path.

I'm not sure the full path is always needed when referencing especially with modern IDEs and code intelligence enabled on GitHub/GitLab?

Well as someone who is rockingvim I appreciate it a lot.

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
Copy link
Member

nejch commentedJun 7, 2024
edited
Loading

Thanks@JohnVillalovos I'd maybe go with a compromise and follow Google's styleguide then, maybe like their example in thedecision chapter:

  • Use import x for importing packages and modules.
  • Use from x import y where x is the package prefix and y is the module name with no prefix.
  • ...

For example the modulesound.effects.echo may be imported as follows:

fromsound.effectsimportecho...echo.EchoFilter(input,output,delay=0.7,atten=4)

Otherwise for example if we wanted to be consistent and add this to our tests as well, we'd have to go with things likeassert isinstance(allowlist, gitlab.v4.objects.AllowlistProjectManager) + the same in function signatures everywhere and I think that would get super verbose pretty quick. Maybe best to have modules split logically so the source of modules is clear at the import section already (in the tests I think we already do a pretty good job with this).

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.

@igorp-collabora
Copy link
Contributor

Be aware thatSOME_MODULE.some_function() will perform an attribute lookup compared to callingsome_function directly. For hot code sections it is probably best to have a direct reference over indirect through the module.

nejch reacted with thumbs up emoji

@JohnVillalovos
Copy link
MemberAuthor

Be aware thatSOME_MODULE.some_function() will perform an attribute lookup compared to callingsome_function directly. For hot code sections it is probably best to have a direct reference over indirect through the module.

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.

@github-actionsGitHub Actions
Copy link

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.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nejchnejchnejch left review comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@JohnVillalovos@nejch@igorp-collabora

[8]ページ先頭

©2009-2025 Movatter.jp