-
-
Notifications
You must be signed in to change notification settings - Fork 12
pre-commit isort hook ignores .isort.cfg, namely skip section #9
Comments
first: moving this to the proper project |
next you'll need to provide a full reproduction -- "doesn't work" isn't a bug report. this should (at a minimum) include a also note that there isn't anything special going on here -- pre-commit just invokes looking forward to your more complete report! |
pre-commit config:
.isort.cfg
#Steps for reproduction: I have a file prepare_data.py in embeddings config I do following steps:
The problem for me is that isort should not touch this file at all (as it stays in skip variable in On second attempt, the modified file is committed (and import order in it is now incorrect) |
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.yamlrepos:
- 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.pyimport os
import astpretty t2.pyimport astpretty
import os runtimethere'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-commitsame files as above, but this time we'll invoke $ 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 |
that said, even though trying this configuration on for size appears to work: .pre-commit-config.yamlrepos:
- 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$
|
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) |
just curious -- why isn't the workaround workable for you? |
Shortly - seems like too complex in case of multiple files. As well, I have found currently, that |
|
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. |
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. 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. |
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):
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
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 |
the in args: [-i, --filter-files] and a that said, I don't use |
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:
becomes
do you know if there are plans to change it to:
(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! |
there is not, the readme outlines this |
that's fair. thanks for the info! I really like how this is not over-engineered, great job! |
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
The text was updated successfully, but these errors were encountered: