canonical / is-charms-contributing-guide

The code contributing guide for the IS charms team
Apache License 2.0
2 stars 1 forks source link

Import module rather than from module #7

Closed jdkandersson closed 2 years ago

jdkandersson commented 2 years ago

Prefer to import the whole module as a namespace rather than importing functions or classes from the module.

merkata commented 2 years ago

Can you expand on that, perhaps provide your thoughts and some links if available? Thanks!

mthaddon commented 2 years ago

I don't think having a blanket rule to always prefer the whole module is always a good way to go. It can lead to unnecessarily long references to functions and means being less explicit in imports about which parts of a class or module you actually need.

In the jenkins agent operator PR you referenced https://google.github.io/styleguide/pyguide.html#22-imports as having more guidance on this, but from my reading they offer a number of different approaches, and even list this as a specific example:

from sound.effects import echo
...
echo.EchoFilter(input, output, delay=0.7, atten=4)
varshigupta12 commented 2 years ago

I think considering importing a module(import example.foo.bar) versus importing a class(import example.foo.bar..FooBarClass) both would import the entire module-> in most cases it makes sense to import just the module. But I am not in favor of a blanket rule to always import a module either as in some cases that'll decrease the code readability(For ex- in cases you have to refer to class FooBar in your code again and again)

I'll prefer a module import when the class name is too generic or in shorthand and a class/function import when it's a more unique name and could be easily referred to without any naming conflicts.

jdkandersson commented 2 years ago

I don't think having a blanket rule to always prefer the whole module is always a good way to go. It can lead to unnecessarily long references to functions and means being less explicit in imports about which parts of a class or module you actually need.

In the jenkins agent operator PR you referenced https://google.github.io/styleguide/pyguide.html#22-imports as having more guidance on this, but from my reading they offer a number of different approaches, and even list this as a specific example:

from sound.effects import echo
...
echo.EchoFilter(input, output, delay=0.7, atten=4)

The from sound.effects import echo is still consistent with the initial intent since it ends up importing the echo module (although it is a submodule) rather than the EchoFilter class, it wasn't clear in the issue description above that this was the intent.

If I summarise the feedback so far:

Recommendation is to usually import the module or one-up sub-module instead of a function or class from that module for clarity on where a class or function has come from. Examples of exceptions are:

Is that a reasonable summary?

arturo-seijas commented 2 years ago

I don't think having a blanket rule to always prefer the whole module is always a good way to go. It can lead to unnecessarily long references to functions and means being less explicit in imports about which parts of a class or module you actually need. In the jenkins agent operator PR you referenced https://google.github.io/styleguide/pyguide.html#22-imports as having more guidance on this, but from my reading they offer a number of different approaches, and even list this as a specific example:

from sound.effects import echo
...
echo.EchoFilter(input, output, delay=0.7, atten=4)

The from sound.effects import echo is still consistent with the initial intent since it ends up importing the echo module (although it is a submodule) rather than the EchoFilter class, it wasn't clear in the issue description above that this was the intent.

If I summarise the feedback so far:

Recommendation is to usually import the module or one-up sub-module instead of a function or class from that module for clarity on where a class or function has come from. Examples of exceptions are:

  • When a class or function is used repeatedly in a file
  • It is commonly used in a given ecosystem, such as functions and classes from the ops module for charms
  • The class or function is well named and it is clear where it came from

Is that a reasonable summary?

I'd like to add some thoughts to expand on what has already been mentioned in relation with code verbosity with this approach.

I don't think this approach has any real advantage since the IDE already points to the class definition if needed. Moreover, automatic imports will use the from ... import format whenever adding an import for a new construct. This will add considerable overhead by having to manually modify most of the imports.

jdkandersson commented 2 years ago

Something else to consider, the zen of Python includes a reference to namespaces at the bottom: https://peps.python.org/pep-0020/

mthaddon commented 2 years ago

If I summarise the feedback so far:

Recommendation is to usually import the module or one-up sub-module instead of a function or class from that module for clarity on where a class or function has come from. Examples of exceptions are:

  • When a class or function is used repeatedly in a file
  • It is commonly used in a given ecosystem, such as functions and classes from the ops module for charms
  • The class or function is well named and it is clear where it came from

Is that a reasonable summary?

I think that's a useful summary. However, it seems there are quite a few exceptions, and having this rule might be less useful as a result. I also agree with Arturo's comment about IDEs already pointing to class definitions if needed. To me the import statements themselves are what helps you identify where something came from. My personal preference is to be more explicit in imports - if you plan to use specific parts of a module, import them rather than the whole module, as I feel like that helps me understand more accurately what my code depends on.

jdkandersson commented 2 years ago

Let's park this one then