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

isort is (incorrectly) formatting code outside of imports #2315

Closed
reidswanson opened this issue Dec 20, 2024 · 8 comments
Closed

isort is (incorrectly) formatting code outside of imports #2315

reidswanson opened this issue Dec 20, 2024 · 8 comments

Comments

@reidswanson
Copy link

Given this snippet of code in my alembic/env.py file:

import logging
import re
from logging.config import fileConfig
from sqlalchemy import engine_from_config, pool
from alembic import context


USE_TWOPHASE = False

# this is the Alembic Config object, which provides
# access to the values within the .ini file in use.
config = context.config

# Interpret the config file for Python logging.
# This line sets up loggers basically.
if config.config_file_name is not None:
    fileConfig(config.config_file_name)

logger = logging.getLogger("alembic.env")

# gather section names referring to different
# databases.  These are named "engine1", "engine2"
# in the sample .ini file.
db_names = config.get_main_option("databases", "")

# add your model's MetaData objects here
# for 'autogenerate' support.  These must be set
# up to hold just those tables targeting a
# particular database. table.tometadata() may be
# helpful here in case a "copy" of
# a MetaData is needed.
# from myapp import mymodel
# target_metadata = {
#       'engine1':mymodel.metadata1,
#       'engine2':mymodel.metadata2
# }
target_metadata = {}


# other values from the config, defined by the needs of env.py,
# can be acquired:
# my_important_option = config.get_main_option("my_important_option")
# ... etc.
def run_migrations_offline() -> None:
    ...

After running isort, it produces this output:

# Standard Library
import logging
import re

from logging.config import fileConfig

# 3rd Party Library
from sqlalchemy import engine_from_config, pool

# 1st Party Library
from alembic import context


USE_TWOPHASE = False
# this is the Alembic Config object, which provides
# access to the values within the .ini file in use.
config = context.config
# Interpret the config file for Python logging.
# This line sets up loggers basically.
if config.config_file_name is not None:
    fileConfig(config.config_file_name)

logger = logging.getLogger("alembic.env")
# gather section names referring to different
# databases.  These are named "engine1", "engine2"
# in the sample .ini file.
db_names = config.get_main_option("databases", "")
# add your model's MetaData objects here
# for 'autogenerate' support.  These must be set
# up to hold just those tables targeting a
# particular database. table.tometadata() may be
# helpful here in case a "copy" of
# a MetaData is needed.
# from myapp import mymodel
# target_metadata = {
#       'engine1':mymodel.metadata1,
#       'engine2':mymodel.metadata2
# }
target_metadata = {}
# other values from the config, defined by the needs of env.py,
# can be acquired:
# my_important_option = config.get_main_option("my_important_option")
# ... etc.
def run_migrations_offline() -> None:

It mostly organizes the imports correctly (although I'm not sure why sqlalchemy is 3rd party and alembic is 1st party), but it removes several desired and PEP compliant blank lines outside of the import section.

I am using isort 5.13.2 with the following configuration options set in my pyproject.toml

[tool.isort]
profile = "black"
lines_before_imports = 1
lines_after_imports = 2
lines_between_types = 1
import_heading_future = "Future Library"
import_heading_stdlib = "Standard Library"
import_heading_thirdparty = "3rd Party Library"
import_heading_firstparty = "1st Party Library"
import_heading_localfolder = "Project Library"
force_alphabetical_sort_within_sections = true
@MajorTanya
Copy link

MajorTanya commented Dec 25, 2024

although I'm not sure why sqlalchemy is 3rd party and alembic is 1st party

Stumbled across this issue and I think I can at least unravel this mystery. I think this happens because "alembic" resolves to both the package as well as the folder it generates when you initialise it. I guess isort sees that "alembic" resolves to a local directory and places it with the other first party imports.

@reidswanson
Copy link
Author

Stumbled across this issue and I think I can at least unravel this mystery. I think this happens because "alembic" resolves to both the package as well as the folder it generates when you initialise it. I guess isort sees that "alembic" resolves to a local directory and places it with the other first party imports.

Thanks for the reply and that makes a lot of sense. However, the much bigger issue is that isort is completely messing up the formatting of the blank lines.

@MaxymVlasov
Copy link

I not sure is it related or fully separate issue, but pre-commit.com hook with

- repo: https://github.com/pycqa/isort
  rev: 5.13.2
  hooks:
    - id: isort
      name: isort
      args: [--force-single-line, --profile=black]

in next code, previously formatted by ruff

def test_subcommand_modules(mocker):
    from pre_commit_terraform._cli_subcommands import (
        SUBCOMMAND_MODULES as patched_subcommand_modules,
    )

replace indentation level from 4 spaces to 2:

     from pre_commit_terraform._cli_subcommands import (
-        SUBCOMMAND_MODULES as patched_subcommand_modules,
+      SUBCOMMAND_MODULES as patched_subcommand_modules,
     )

Similar happens with args: [--force-single-line] and with no args (identical diff):

 def test_subcommand_modules(mocker):
-    from pre_commit_terraform._cli_subcommands import (
-        SUBCOMMAND_MODULES as patched_subcommand_modules,
-    )
+    from pre_commit_terraform._cli_subcommands import \
+      SUBCOMMAND_MODULES as patched_subcommand_modules

@webknjaz
Copy link

def test_subcommand_modules(mocker):
    from pre_commit_terraform._cli_subcommands import (

To be fair, having imports in function is a bad idea regardless of whether a formatter would get it or not…

@reidswanson
Copy link
Author

reidswanson commented Mar 3, 2025

I've been trying out VSCode (instead of PyCharm) with the isort extension. However, it also completely messes up the formatting of my code. As in my first example, it removes all blank lines between comments and expressions throughout the entire file. I am not using it with black (as suggested in the extension instructions) and I have tried it with Ruff enabled and disabled with no difference.

I should also note, running isort manually, >isort path/to/my/file.py, also produces the same incorrect formatting.

@reidswanson
Copy link
Author

I think I've finally narrowed it down. Changing the value of lines_before_imports is what triggers the issue. Using the default value (omitting it from the configuration) prevents the behavior that I am seeing.

@staticdev
Copy link
Collaborator

I not sure is it related or fully separate issue, but pre-commit.com hook with

  • repo: https://github.com/pycqa/isort
    rev: 5.13.2
    hooks:
    • id: isort
      name: isort
      args: [--force-single-line, --profile=black]
      in next code, previously formatted by ruff

def test_subcommand_modules(mocker):
from pre_commit_terraform._cli_subcommands import (
SUBCOMMAND_MODULES as patched_subcommand_modules,
)
replace indentation level from 4 spaces to 2:

 from pre_commit_terraform._cli_subcommands import (
  •    SUBCOMMAND_MODULES as patched_subcommand_modules,
    
  •  SUBCOMMAND_MODULES as patched_subcommand_modules,
    
    )
    Similar happens with args: [--force-single-line] and with no args (identical diff):

def test_subcommand_modules(mocker):

  • from pre_commit_terraform._cli_subcommands import (
  •    SUBCOMMAND_MODULES as patched_subcommand_modules,
    
  • )
  • from pre_commit_terraform._cli_subcommands import \
  •  SUBCOMMAND_MODULES as patched_subcommand_modules
    

ruff has a partial implementation with isort, it is mostly a convergence issue and mostly needs to have similar logic on ruff side for them to be compatible.

Other than that I believe @reidswanson found the solution to his problem. Closing this one.

@reidswanson
Copy link
Author

FWIW, should this issue really be closed? This clearly seems like a bug. Even though I can work around it by not using the parameter, isort should not be modifying code outside of the import section regardless of configuration settings.

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

No branches or pull requests

5 participants