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 does not skip files #885

Closed
nikitagashkov opened this issue Mar 6, 2019 · 29 comments
Closed

isort does not skip files #885

nikitagashkov opened this issue Mar 6, 2019 · 29 comments
Labels
bug Something isn't working

Comments

@nikitagashkov
Copy link

nikitagashkov commented Mar 6, 2019

Describe the bug

isort does not respect skip and skip_glob configuration options.

To Reproduce

Steps to reproduce the behavior:

  1. Create file.py with the following content:
    import os, \
        sys
  2. Create .isort.cfg with the following content:
    [settings]
    skip_glob = file.py
    
  3. Run isort file.py.
  4. Open file.py and check out it's content to become:
    import os
    import sys

Expected behavior

A file.py is skipped and is not touched.

Screenshots

Environment (please complete the following information):

  • OS: Linux
  • Python version: 3.7.2
  • isort version: 4.3.11

Additional context

isort 4.3.10 is working correctly. Issue can be reproduced with skip option as well.

@timothycrosley
Copy link
Member

Hi @nickgashkov,

Can you tell me more about your use case for this?

The updated behavior basically makes files you explicitly pass from the command line, assumed non skipped - in all other cases they are skipped (such as when doing recursive sorting).

If you do not explicitly pass in that file, but rather a directory it will be skipped. If you pass it in using the API it will also be skipped. I'm curious about the use case where you want to skip files based on a config, but then explicitly pass them in? The reason for this change was that with the old behavior if you say, skipped "build" because you had a build directory inside your project, but then ran in travis which put all your code inside a build directory - you would suddenly be skipping all your files unintentionally.

@nikitagashkov
Copy link
Author

Hello @timothycrosley,

Thanks for a fast reply. Sorry for unclear description, I didn't know about the behavior of explicitly passed files. The issue can be reproduced with isort -rc . as well.

Given the following project structure...

image

...and the contents of file.py as in the original issue description and .isort.cfg contents like:

[settings]
skip_glob = **/*.py

isort 4.3.11 does not skip file.py while isort 4.3.10 does.

I've tested the following config...

[settings]
skip_glob = *.py

...and the file.py is correctly skipped. Maybe something's wrong with the glob...

@silasary
Copy link

silasary commented Mar 6, 2019

Following up on this, we have an .isort.cnf in a subfolder of our project, as we only want to skip a file in that particular directory.

Moving the .isort.cfg to the root folder fixes the problem, but incidentally skips a similarly named file in a different subdirectory.

This setup worked with 4.3.4 but not 4.3.11

@brianmay
Copy link
Contributor

brianmay commented Mar 6, 2019

I think, rightly or wrongly, skip_glob only matches the filename, not the complete path. Meaning components like */ or **/ don't work right now - and are not required either.

@brianmay
Copy link
Contributor

brianmay commented Mar 6, 2019

I think the skip option might also be only matching the filename, however I am not sure if this is expected behavior or not. e.g.

skip=migrations

Will skip all directories called migrations not just one directory at the top level. Looking at other examples would suggest it is only suppose to match the exact directory I specified.

@brianmay
Copy link
Contributor

brianmay commented Mar 7, 2019

There does appear to be a regression here. In version 4.3.10 the following works:

skip_glob = */migrations/*.py

In version 4.3.11 it now processes files such as karaage/migrations/0002_auto_20140924_1111.py. Will try to narrow it down to the commit.

@timothycrosley timothycrosley added the bug Something isn't working label Mar 7, 2019
@brianmay
Copy link
Contributor

brianmay commented Mar 7, 2019

Forget what I just said (and deleted). Me confused.

First bad commit is 2c59158 in #878.

Note this commit was hard to test, because it contained unrelated errors fixed in subsequent commits.

brianmay added a commit to Karaage-Cluster/karaage that referenced this issue Mar 7, 2019
Eric-Arellano added a commit to pex-tool/pex that referenced this issue Mar 7, 2019
## Problem
isort had a regression in 4.3.11 that it no longer respects `skip_glob` as we intended. See PyCQA/isort#885.

This motivated the problem, although there is reason to pin isort regardless of this regression. Linters and autoformatters often change their behavior with new releases. When this happens, we don't want someone's PR to fail due to an unrelated upstream change in that tool's behavior.

## Solution
Pin isort to 4.3.10, the most recent release that does not have the `skip_glob` issue. Once this issue gets resolved, the version can be upgraded.

Also fix bad imports that weren't caught with our original isort runs.

Finally, convert CLI flags to use long names.
@timothycrosley
Copy link
Member

I'm sorry for letting this regression slip in the last release! It took a while for me to untangle, but it should be fixed in the latest release (4.3.13) of isort, with additional testing put in place to try to ensure the regression is not reintroduced on the future.

Thanks!

~Timothy

@brianmay
Copy link
Contributor

brianmay commented Mar 8, 2019

Extra testing -> sounds great!

@brianmay
Copy link
Contributor

brianmay commented Mar 8, 2019

Unfortunately doesn't look good :-(. I have:

skip_glob = */migrations/*.py

Version 4.3.13 (unlike 4.3.10) wants to do files such as karaage/migrations/0001_initial.py.

@timothycrosley
Copy link
Member

Hi @brianmay,

Sorry to hear that! This case should be fixed as well in 4.3.14, with additional testing in place as well :)

https://github.com/timothycrosley/isort/releases/tag/4.3.14

Thanks!

~Timothy

@brianmay
Copy link
Contributor

So far that does look a lot better now. Thanks. Have tested with one project, will test with another (more complicated) project tomorrow.

@silasary
Copy link

I'm still having issues with an .isort.cfg in a subdirectory.

The config is located (relative to the project root) at /decksite/charts/.isort.cfg

with the contents:

[settings]
skip=chart.py

@brianmay
Copy link
Contributor

@silasary Are you saying it isn't skipping the chart.py file? Is this file in the same directory as the .isort.cfg file?

@silasary
Copy link

Correct. The file is in the same subdirectory as the config file.

@brianmay
Copy link
Contributor

@silasary I think isort only looks for .isort.cfg files in the current working directory. I just did a test against version 4.3.10 and it also ignores appears to ignore .isort.cfg files in sub-directories. I don't see anything in the documentation that says otherwise either.

@bakert
Copy link

bakert commented Mar 16, 2019

[gaghalfrunt pd] find . -name .isort.cfg
./decksite/charts/.isort.cfg
[gaghalfrunt pd] cat ./decksite/charts/.isort.cfg
[settings]
skip=chart.py

4.3.12 and up:

[gaghalfrunt pd] isort --check-only
ERROR: /Users/bakert/pd/decksite/charts/chart.py Imports are incorrectly sorted.

4.3.11 has a whole other behavior where lots of files are flagged I assume that's a bad release.

4.3.10 and down:

[gaghalfrunt pd] isort --check-only
Skipped 8 files

Why 8 files? I don't know. It was "Skipped 1 file" before I started flipping versions around. I've butted my head against this a little to try and provide a saner report but I can't figure out why it's 8.

But it's definitely true that the behavior changes between versions re: .isort.cfg. Any hints on why it's 8 files not 1 file lmk and I'll try and clarify what's happening here.

@bakert
Copy link

bakert commented Mar 16, 2019

Moving the .isort.cfg to the root/working directory and changing it to:

[settings]
skip=decksite/charts/chart.py

gives "Skipped 3 files" with 4.3.15.

Still confused but maybe that's good enough.

@brianmay
Copy link
Contributor

Using the verbose mode, -vb might help.It should list the files that are skipped.

@bakert
Copy link

bakert commented Mar 17, 2019

Thanks.

WARNING: .mypy_cache was skipped as it's listed in 'skip' setting or matches a glob in 'skip_glob' setting
WARNING: .git was skipped as it's listed in 'skip' setting or matches a glob in 'skip_glob' setting
WARNING: chart.py was skipped as it's listed in 'skip' setting or matches a glob in 'skip_glob' setting
Skipped 3 files

OK so I guess we are all good with the new behavior as long as the old behavior was unintentional/doesn't need to be supported.

For interest's sake the 8 files skipped under 4.3.10 were:

WARNING: chart.py was skipped as it's listed in 'skip' setting or matches a glob in 'skip_glob' setting
WARNING: 3.7 was skipped as it's listed in 'skip' setting or matches a glob in 'skip_glob' setting
WARNING: 3.6 was skipped as it's listed in 'skip' setting or matches a glob in 'skip_glob' setting
WARNING: objects was skipped as it's listed in 'skip' setting or matches a glob in 'skip_glob' setting
WARNING: info was skipped as it's listed in 'skip' setting or matches a glob in 'skip_glob' setting
WARNING: logs was skipped as it's listed in 'skip' setting or matches a glob in 'skip_glob' setting
WARNING: hooks was skipped as it's listed in 'skip' setting or matches a glob in 'skip_glob' setting
WARNING: refs was skipped as it's listed in 'skip' setting or matches a glob in 'skip_glob' setting
Skipped 8 files

@GeyseR
Copy link
Contributor

GeyseR commented Apr 19, 2019

Hi @timothycrosley

I'm curious about the use case where you want to skip files based on a config, but then explicitly pass them in?

we rely on this behavior in our project. Our use case is next:

  1. in our configuration file we have the following path: */migrations/* to skip imports codestyle check for auto-generated migrations all the time
  2. we have automation script for tracking changes between the current and previous successful build and run isort only for those specific files. Any changelog might contain any number of files including migrations, but we still want to skip migration during codestyle check. So, unfortunately, described changes introduce a regression for our case :(

Does that make sense?

@jparise
Copy link
Member

jparise commented May 17, 2019

This new "never skip when passed a filename" behavior is also a regression in our environment, where we wrap isort calls in a linter framework that works on a per-file basis.

I can also think of cases where isort might be called as part of an auto-formatting editor hook, in which case you would want it to respect a project-wide set of "skips".

A compromise could be a command line option that would opt-in or opt-out of this new behavior.

@timothycrosley
Copy link
Member

@jparise,

Completely agree! I believe 4.3.18 may have included the exact flag that you are hoping for:

https://github.com/timothycrosley/isort/blob/develop/CHANGELOG.md#4318---may-1-2019---hot-fix-release:

--filter-files

Thanks!

~Timothy

@pySilver
Copy link

pySilver commented Nov 8, 2019

Oh, it drives me mad :)

My setup.cfg located in ~/Projects/foo with the following contents:

[flake8]
max-line-length = 120
exclude = .tox,.git,*/migrations/*,*/static/CACHE/*,docs,node_modules

[pycodestyle]
max-line-length = 120
exclude = .tox,.git,*/migrations/*,*/static/CACHE/*,docs,node_modules

[mypy]
python_version = 3.7
check_untyped_defs = True
ignore_errors = False
ignore_missing_imports = True
strict_optional = True
warn_unused_ignores = True
warn_redundant_casts = True
warn_unused_configs = True

[mypy-*.migrations.*]
# Django migrations should not produce any errors:
ignore_errors = True

[tool:isort]
line_length=120
multi_line_output=3
include_trailing_comma=True
skip=migrations
skip_glob=*/node_modules/*,*/config/settings/*.py
known_first_party=myprojectnamehere
known_third_party=willow,modelcluster,taggit,unidecode,bs4,pytz,PIL
known_django=django
known_wagtail=wagtail
sections=FUTURE,STDLIB,DJANGO,WAGTAIL,THIRDPARTY,FIRSTPARTY,LOCALFOLDER
default_section=THIRDPARTY

There is no .isort.cfg anywhere and neither .editorconfig contains anything related to isort.

Observed problem: Files within ~/Projects/foo/config/settings/ are not skipped. I've tried several combinations and it never works:

isort --check-only ~/Projects/foo/config/settings/base.py -vb --settings-path /Users/Silver/Projects/foo/

/#######################################################################\
....skipped
                            VERSION 4.3.21

\########################################################################/

from-type place_module for django.utils.translation returned DJANGO
else-type place_module for environ returned THIRDPARTY
SUCCESS: /Users/Silver/Projects/foo/config/settings/base.py Everything Looks Good!

isort version: 4.3.21
@timothycrosley what I'm doing wrong?

@timothycrosley
Copy link
Member

I believe the following should work for you: isort --check-only ~/Projects/foo/config/settings/base.py -vb --settings-path /Users/Silver/Projects/foo/ --filter-files

@pySilver
Copy link

pySilver commented Nov 9, 2019

oh, yeah --filter-files helps. I didn't expect explicit path overrides settings

@pySilver
Copy link

pySilver commented Nov 9, 2019

Thanks for the tool, its great!

@anilbey
Copy link

anilbey commented Apr 13, 2022

Hi I also got the same confusions.
Why isn't --filter-files True by default?

@gareth-leake
Copy link

gareth-leake commented Apr 28, 2022

Running into a similar problem. I am trying to use pre-commit with isort. The problem is that if I have a file that I want isort to ignore, but then I make a change to that file, the changed file is passed (via pre-commit) to isort, and thus not ignored. Is there any solution here? I've tried passing --filter-files without any success.

Thanks for your help, @timothycrosley

UPDATE:

I think I finally got a working solution. Below is my .pre-commit.config.yaml that successfully ignores desired files.

-   repo: https://github.com/pycqa/isort
    rev: 5.10.1
    hooks:
    -   id: isort
        name: isort (python)
        args: [--skip=<FILE_NAME_HERE>, --filter-files]

UPDATE 2: never mind, after further trial and error, still no luck here.

Final Resolution: Ended up using reorder-python-imports, from the creator of pre-commit. Would love to see a solve to this, as I personally prefer the import style of isort.

adam-grant-hendry pushed a commit to adam-grant-hendry/pyvistaqt that referenced this issue Jun 28, 2022
`pre-commit` is known to override behavior from config files in
certain instances. This commit fixes known issues for:

- `isort`
- `black`
- `mypy`

For relevant details, see:

isort:
  + https://jugmac00.github.io/blog/isort-and-pre-commit-a-friendship-with-obstacles/
  + PyCQA/isort#885

black:
  + psf/black#1584

mypy:
  + python/mypy#4008 (comment)
  + https://pre-commit.com/#hooks-pass_filenames
adam-grant-hendry pushed a commit to adam-grant-hendry/pyvistaqt that referenced this issue Jun 28, 2022
`pre-commit` is known to override behavior from config files in
certain instances. This commit fixes known issues for:

- `isort`
- `black`
- `mypy`
- `pydocstyle`

For relevant details, see:

isort:
  + https://jugmac00.github.io/blog/isort-and-pre-commit-a-friendship-with-obstacles/
  + PyCQA/isort#885

black:
  + psf/black#1584

mypy/pydocstyle:
  + python/mypy#4008 (comment)
  + https://pre-commit.com/#hooks-pass_filenames
akaszynski pushed a commit to pyvista/pyvistaqt that referenced this issue Jun 29, 2022
* refactor(trove-classifiers): update to python 3.7-3.9

* refactor(linters): add `.flake8` and `.codespellrc` configuration files

Also add `PullRequest` to `ignore_words.txt`

* feat(pyproject): add `pyproject.toml`

Currently, this is only use for configuring linters and formatters (no
build file specifications are set). Configuration options for `black`
and `pydocstyle` are added here in this commit.

* feat(pre-commit): add pre-commit

* fix(mypy): add `mypy` dependency to `environment.yml`

This supports conda test/build environments.

* feat(pylint): Update `.pylintrc`

Make this match settings specififed in `Makefile`.

* feat(test): align checks

All checks are placed in appropriate config files. Test runner scripts
point to these so that there exists a single source of truth for all
configurations and that the tests performed across systems is the same.
This may be adjusted, of course, if different settings must be used on
a per-system basis.

* feat(requirements): add `toml`

This package supports reading `pyproject.toml`, which is required for
some of our tests.

* fix(pre-commit): fix file passing errors

`pre-commit` is known to override behavior from config files in
certain instances. This commit fixes known issues for:

- `isort`
- `black`
- `mypy`
- `pydocstyle`

For relevant details, see:

isort:
  + https://jugmac00.github.io/blog/isort-and-pre-commit-a-friendship-with-obstacles/
  + PyCQA/isort#885

black:
  + psf/black#1584

mypy/pydocstyle:
  + python/mypy#4008 (comment)
  + https://pre-commit.com/#hooks-pass_filenames

* feat(ignores): ignore specific linting errors

Ignore linting errors in source code in this PR. The purpose of this PR
is to update linting tools. A future PR will correct the errors. This is
done to separate concerns.

Co-authored-by: Hendry, Adam <adam.hendry@medtronic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

10 participants