Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The lint and fix subcommands disagree about whether to split imports #97

Closed
bkeryan opened this issue Jan 6, 2023 · 0 comments · Fixed by #102
Closed

The lint and fix subcommands disagree about whether to split imports #97

bkeryan opened this issue Jan 6, 2023 · 0 comments · Fixed by #102

Comments

@bkeryan
Copy link
Contributor

bkeryan commented Jan 6, 2023

The lint subcommand does not warn about combined imports like from os import path, access, but the fix subcommand splits them into multiple lines anyway. However, it only does this when there are lint warnings. If there are no lint warnings, fix leaves the combined imports alone.

I suggest configuring fix to use/preserve combined imports, because they are more concise and I don't think they hurt readability. However, it would also be reasonable to update lint to warn about combined imports.

An example from ni-measurementlink-service

# before
from typing import Iterable, List, NamedTuple, Optional

from ni_measurementlink_service._internal.stubs.ni.measurementlink.sessionmanagement.v1 import (
    session_management_service_pb2,
    session_management_service_pb2_grpc,
)
# after
from typing import Iterable
from typing import List
from typing import NamedTuple
from typing import Optional

from ni_measurementlink_service._internal.stubs.ni.measurementlink.sessionmanagement.v1 import (
    session_management_service_pb2,
)
from ni_measurementlink_service._internal.stubs.ni.measurementlink.sessionmanagement.v1 import (
    session_management_service_pb2_grpc,
)

Steps to reproduce

tests\test_cli\fix_test_cases__snapshots\basic_example\input.py demonstrates the behavior.

  1. Run copy .\tests\test_cli\fix_test_cases__snapshots\basic_example\input.py .
  2. Run poetry run ni-python-styleguide lint input.py. None of the warnings are about combined imports:
PS D:\dev\python-styleguide> poetry run ni-python-styleguide lint input.py
input.py:7:1: I201 Missing newline between import groups. 'import pytest' is identified as Third Party and 'from typing import Iterable, List, Hashable' is identified as Stdlib.
input.py:8:1: I100 Import statements are in the wrong order. 'import pathlib' should be before 'import pytest' and in a different group.
input.py:8:1: I201 Missing newline between import groups. 'import pathlib' is identified as Stdlib and 'import pytest' is identified as Third Party.
  1. Manually fix the warnings in input.py:
import pathlib
from os import path, access
from typing import (
    Iterable,
    List,
    Hashable,  # noqa F401: un-used import comment that is actually used, should get removed in --aggressive (auto-generated noqa)
)

import pytest
  1. Run poetry run ni-python-styleguide lint input.py. There are no warnings.
  2. Run poetry run ni-python-styleguide fix input.py. It does not make any changes.
  3. Run copy .\tests\test_cli\fix_test_cases__snapshots\basic_example\input.py input2.py
  4. Run poetry run ni-python-styleguide fix input2.py. It splits the imports:
PS D:\dev\python-styleguide> poetry run ni-python-styleguide fix input2.py
All done! ✨ 🍰 ✨
1 file left unchanged.
All done! ✨ 🍰 ✨
1 file left unchanged.
WARNING:flake8.options.manager:option --indent-size: please update `help=` text to use %(default)s instead of %default -- this will be an error in the future
WARNING:flake8.options.manager:option --max-complexity: please update from optparse string `type=` to argparse callable `type=` -- this will be an error in the future
WARNING:flake8.checker:The multiprocessing module is not available. Ignoring --jobs arguments.
import pathlib
from os import access
from os import path
from typing import (  # noqa F401: un-used import comment that is actually used, should get removed in --aggressive (auto-generated noqa)
    Hashable,
)
from typing import Iterable
from typing import List

import pytest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant