Skip to content

Conversation

@cognifloyd
Copy link
Member

Before this change, some places explicitly imported models from st2client.models.<model> and other places imported models from st2client.models. These st2client.models imports rely on the fact that st2client/st2client/models/__init__.py implicitly imports all models like:

from st2client.models.<model> import *  # noqa

Note the "# noqa" which silences our linters instead of dealing with the bad import * practice.

For external code, relying on the from ... import * in st2client.models is fine; And, doing so allows 3rd party code to have a much nicer interface. However, in ST2 code we should prefer explicit imports.

Before this change, some places explicitly imported models from
`st2client.models.<model>` and other places imported models from
`st2client.models`. These `st2client.models` imports rely on the fact
that `st2client/st2client/models/__init__.py` implicitly imports all
models like: `from st2client.models.<model> import *  # noqa`
Note the "# noqa" which silences our linters instead of dealing with
the bad `import *` practice.

For external code, relying on the `from ... import *` in `st2client.models`
is fine; And, doing so allows 3rd party code to have a much nicer
interface. However, in ST2 code we should prefer explicit imports.
@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Aug 18, 2021
@cognifloyd cognifloyd added this to the 3.6.0 milestone Aug 18, 2021
Copy link
Member

@Kami Kami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for no star imports :)

from six.moves import range

from st2client import models
from st2client.models.action import Action, Execution
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought black forces import per line, but maybe that's not the case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For importing packages or modules (import ...), yes it is one import per line as in PEP 8, but not for importing something from a module (from ... import ...).

I find splitting the from ... import ... imports across multiple lines with the same from ... import statement on each line to be overly verbose. If there are too many to fit on one line, then I go for this syntax (assuming it works with the project's code style):

from some.package.module import (
    MY_CONSTANT,
    my_function,
    MyClass,
    something_else,
    another_something_else,
)

I'm not sure if isort or some other tool could enforce that style, but that wouldn't be part of this PR, however we do it.

These answers discuss PEP 8 and import styling:
https://stackoverflow.com/a/15011456
https://stackoverflow.com/a/38427828

@cognifloyd cognifloyd enabled auto-merge August 31, 2021 06:49
@cognifloyd cognifloyd merged commit 6df04da into StackStorm:master Aug 31, 2021
cognifloyd added a commit that referenced this pull request Nov 11, 2021
Follow code style introduced in #5333
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component:st2client pantsbuild refactor size/M PR that changes 30-99 lines. Good size to review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants