pre-commit isort hook ignores .isort.cfg, namely skip section

[serhiy-yevtushenko]

ISort allows specifying in commit in skip section of configuration the files, which should be excluded from sorting imports.

However, when such file was modified due to different reason (for example, doc string reformating), and is being processed by pre-commit, then isort reformats this file, and ignores, that it is being marked to be skipped by isort in config (I observed this behaviour when using isort in combination with seed-isort-config

This makes isort pre-commit hook unusable for my project

[asottile]

first: moving this to the proper project

[asottile]

next you'll need to provide a full reproduction -- "doesn't work" isn't a bug report. this should (at a minimum) include a pre-commit configuration, some sample files, and your isort configuration.

also note that there isn't anything special going on here -- pre-commit just invokes isort so you're probably seeing isort not reading your configuration. that said, even though I don't use isort myself, I can probably take a look into the issue on isort's side

looking forward to your more complete report!

[serhiy-yevtushenko]

pre-commit config:

repos:
-   repo: https://github.com/asottile/seed-isort-config
    rev: v1.9.1
    hooks:
    -   id: seed-isort-config

-   repo: https://github.com/pre-commit/mirrors-isort
    rev: v4.3.20
    hooks:
    -   id: isort

-   repo: https://github.com/humitos/mirrors-autoflake.git
    rev: v1.1
    hooks:
        - id: autoflake
          args: ['--in-place', '--remove-unused-variable']

-   repo: https://github.com/ambv/black
    rev: stable
    hooks:
    - id: black
      args: [--line-length, "120", "."]

-   repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v1.2.3
    hooks:
    - id: trailing-whitespace
    - id: end-of-file-fixer
    - id: check-json
      files: \.(json)$
    - id: check-yaml
    - id: fix-encoding-pragma
      args: [--remove]
    - id: flake8
      additional_dependencies: [flake8-blind-except, flake8-builtins, flake8-rst-docstrings, flake8-logging-format]

-   repo: https://github.com/pre-commit/mirrors-mypy
    rev: v0.711
    hooks:
    -   id: mypy

-   repo: https://github.com/myint/docformatter
    rev: v1.1
    hooks:
    - id: docformatter
      name: docformatter
      description: 'Formats docstrings to follow PEP 257.'
      entry: docformatter
      args: [-i, --wrap-summaries, "120", --wrap-description, "120"]
      language: python
      types: [python]

-   repo: https://github.com/pycqa/pydocstyle
    rev: 4.0.0
    hooks:
    - id: pydocstyle
      name: pydocstyle
      description: pydocstyle is a static analysis tool for checking compliance with Python docstring conventions.
      entry: pydocstyle
      language: python
      types: [python]

.isort.cfg

[settings]
line_length=120
skip=embeddings/compute_similarities_parts2GAs.py, embeddings/prepare_data.py, embeddings/similarity_model_training_parts2GAs.py, embeddings/word_embedding_training_parts2GAs.py
known_first_party=tautils,parts_2_ga
known_third_party=elasticsearch,fuzzywuzzy,gensim,glove,keras,mock,more_itertools,numpy,pandas,pylint,scipy,setuptools,tensorflow
indent='    '
multi_line_output=0
sections = STDLIB,THIRDPARTY,FIRSTPARTY,LOCALFOLDER

#Steps for reproduction:

I have a file prepare_data.py in embeddings config

I do following steps:

1. Modify formatting in data via docformmatter:
   (one could go to embedding folder and execute:
   docformatter -i --wrap-summaries 120 --wrap-description 120 prepare_data.py)

2. Try to commit file:
   On first attempt, two pre-commit hooks fail (black and isort)

The problem for me is that isort should not touch this file at all (as it stays in skip variable in .isort.cfg)

On second attempt, the modified file is committed (and import order in it is now incorrect)

[asottile]

It's often useful in computing to boil down a complicated reproduction to a "minimal reproducible (compilable) example" -- your example doesn't really include enough information to reproduce exactly so I tried to come up with my own case separately.

minimal case involving pre-commit

.pre-commit-config.yaml

repos:
-   repo: https://github.com/asottile/seed-isort-config
    rev: v1.9.1
    hooks:
    -   id: seed-isort-config
-   repo: https://github.com/pre-commit/mirrors-isort
    rev: v4.3.21
    hooks:
    -   id: isort

.isort.cfg

[settings]
known_third_party = astpretty
skip=t2.py

t.py

import os

import astpretty

t2.py

import astpretty
import os

runtime

there's nothing special about other hooks making modifications, I suspect if you isolate your case you'll find that black has nothing to do with it (?) anyway here's what I have:

$ pre-commit  run --all-files
seed isort known_third_party.............................................Passed
isort....................................................................Failed
hookid: isort

Files were modified by this hook. Additional output:

Fixing /tmp/x/t2.py

but let's try and narrow that down further since pre-commit isn't doing anything special here...

reproduction without pre-commit

same files as above, but this time we'll invoke isort directly:

$ isort -i t.py t2.py
Fixing /tmp/x/t2.py
$ git diff
diff --git a/t2.py b/t2.py
index 6dd3f37..22dcc45 100644
--- a/t2.py
+++ b/t2.py
@@ -1,2 +1,3 @@
-import astpretty
 import os
+
+import astpretty

so much for skip I guess?

[asottile]

that said, even though isort has a bug here, you can always work around broken exclusion settings by using pre-commit's own settings

trying this configuration on for size appears to work:

.pre-commit-config.yaml

repos:
-   repo: https://github.com/asottile/seed-isort-config
    rev: v1.9.1
    hooks:
    -   id: seed-isort-config
-   repo: https://github.com/pre-commit/mirrors-isort
    rev: v4.3.21
    hooks:
    -   id: isort
        exclude: ^t2\.py$

.isort.cfg

[settings]
known_third_party = astpretty

output

$ pre-commit  run --all-files
seed isort known_third_party.............................................Passed
isort....................................................................Passed

[serhiy-yevtushenko]

Thanks for following-up it to the root cause and suggesting a work around (for my case it does not seem to be workable, so, I have decided to switch to reorder_python_imports)

[asottile]

just curious -- why isn't the workaround workable for you?

[serhiy-yevtushenko]

Shortly - seems like too complex in case of multiple files.

As well, I have found currently, that isort supports following --filter_files flag - see the announcement at the end of PyCQA/isort#909

[asottile]

exclude is a regex so that should still work: exclude: ^(foo\.py|bar\.py|...)$

[serhiy-yevtushenko]

Thanks. I really appreciate your effort, and it is more easily to support simple file pathes, then convert every one of them to regex. In my case, the scenario included several pathes in nested directories. While your solution does function, it increases amount of toll in maintenance for me.

[jrmlhermitte]

So it sounds like there is a bug where skip is ignored when running using precommit. Have there been any plans to fix this? I have the same issue.
Thanks for the very detailed investigation with the great minimal working example by the way!

I'm happy to help in any way if I can. If you happen to know what's been done and approximately where to look I can try to contribute.

[asottile]

yes a few messages above yours

[jrmlhermitte]

Thanks! I didn't see that.

I also found that a multiline regex in the .pre-commit-config.yaml might also be a little easier to deal with (still awkward):

      exclude: |
         (?x)(
         file1.py|
         file2.py
         )

in case it helps anyone else. (In my case, we have quite a few files isort fails on and putting them in a one line regex is unmanageable)

I gave the --filter_files flag a try but no success. I'm likely using it wrong. I tried something like:

    args:
        - |
            --filter_files=file1.py,\
            file2.py,
            file3.py

However, since the above works, I'll stick with it for now and that's fine with me. I just thought I'd supply more details in case it helped anyone.

thanks for the reply

[asottile]

the --filter-files option appears to be poorly/un documented in isort, reading the commit my guess at how you'd use it is:

in .pre-commit-config.yaml

    args: [-i, --filter-files]

and a filter_files setting in .isort.cfg

that said, I don't use isort myself because it is not good at detecting third party dependencies (requiring workarounds like seed-isort-config) and it requires nonzero configuration to get it to play nicely with other formatters (add-trailing-comma, autopep8, black, yapf, etc.) whereas reorder-python-imports has no configuration and is compatible with those other tools

[jrmlhermitte]

Great! reorder-python-imports works for us.

Is there line wrapping? It's not obvious to me right now. If there isn't that is fine. This forces the coder to resolve their imports.

Also, looks like things like:

from foo import one, two, three

becomes

from foo import one
from foo import three
from foo import two

do you know if there are plans to change it to:

from foo import (
    one,
    three,
    two
)

(which makes it a little easier to scan)

Note this is not a big deal for us, as the objective is to reduce diffs during the review process, and this accomplishes this.

thanks again!

[asottile]

there is not, the readme outlines this

[jrmlhermitte]

that's fair. thanks for the info! I really like how this is not over-engineered, great job!