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

per-file-ignores in config file should be treated relative to it #693

Open
asottile opened this issue Apr 3, 2021 · 24 comments
Open

per-file-ignores in config file should be treated relative to it #693

asottile opened this issue Apr 3, 2021 · 24 comments

Comments

@asottile
Copy link
Member

asottile commented Apr 3, 2021

In GitLab by @blueyed on Mar 7, 2019, 07:10

With tests/conftest.py:E402 in setup.cfg, flake8 run in tests will still complain about the error.

I think relative paths in config files should be treated relative to the config file's location.

Using flake8 3.7.7 (mccabe: 0.6.1, pycodestyle: 2.5.0, pyflakes: 2.1.1) CPython 3.7.2 on Linux.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Mar 7, 2019, 08:38

Odd, this does seem to be treated differently than exclude= though they appear to use the same code to check:

https://gitlab.com/pycqa/flake8/blob/88caf5ac484f5c09aedc02167c59c66ff0af0068/src/flake8/style_guide.py#L473-491

https://gitlab.com/pycqa/flake8/blob/88caf5ac484f5c09aedc02167c59c66ff0af0068/src/flake8/checker.py#L173-195

I set up the following in /tmp/x:

t/   # cwd is inside `t/`
    t.py
setup.cfg

The difference appears to be here where the "filename" (actually the per-file-ignore pattern) is being normalized:

https://gitlab.com/pycqa/flake8/blob/88caf5ac484f5c09aedc02167c59c66ff0af0068/src/flake8/style_guide.py#L450

Looking at a log, the value for exclude gets normalized as if it is run from the directory of setup.cfg

flake8.options.config     MainProcess     82 DEBUG    Option "exclude" returned value: 't/t.py'
flake8.options.config     MainProcess     82 DEBUG    't/t.py' has been normalized to ['/tmp/x/t/t.py'] for option "exclude"

Looking at the log for per-file-ignores

flake8.style_guide        MainProcess     98 DEBUG    <StyleGuide [/tmp/x/t/t/t.py]> does not match "/tmp/x/t/t.py"

The fix here is to normalize the paths relative to the config file they're defined in, though I'm not quite sure where to thread that through, will take more of a sit down

Thanks for the report 🎉

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @blueyed on Mar 18, 2019, 02:39

Any more findings by now? (closing old tabs)

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Mar 18, 2019, 07:53

nope, havnen't done anything with this since triaging -- if you want to take a stab at it by all means :)

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Mar 21, 2019, 08:24

mentioned in merge request !311

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @ericvw on May 20, 2019, 20:37

mentioned in merge request !321

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @ericvw on May 20, 2019, 20:43

I created !321, which is an initial minimal stab to get this working.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @pjacock on Aug 8, 2019, 03:47

I wonder if this is part of a larger issue with the path normalisation? i.e. new issue #562.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Aug 8, 2019, 08:18

#562 is unrelated, per-file-ignores is not parsed / normalized at all in the same way as normal options which is what needs fixing

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @ericvw on Aug 29, 2019, 10:11

mentioned in merge request !337

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @ericvw on Aug 31, 2019, 06:38

mentioned in merge request !351

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @gbd.lin on Oct 9, 2019, 05:44

What is the current status of this issue? I'm really looking forward to using some features that will be introduced in 3.8.0, especially --extend-exclude but that release depends on fixing this issue currently.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Oct 9, 2019, 06:00

in process, please don't bump issues like this, use the thumbs up

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @pjacock on Jan 17, 2020, 24:34

#562 has been closed as a duplicate, the same symptoms but wider in scope affecting any path configuration values and sources of config files. I included some examples using a tiny plugin adding a path value.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Apr 27, 2020, 13:01

this is likely to miss 3.8.0, but we will try and address this soon (the work to fix this is large, complicated, and in progress)

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @lewoudar on May 31, 2020, 13:03

Hi everyone,
I'm not sure but I think I have an issue related to this one. I have a file starting like this

from gevent import monkey

monkey.patch_all()  # noqa: E402
from .files import read_mp, write_mp

Under flake8 3.7.9 when I run flake8 <source> I had no issue

But under flake8 3.8.2, No I have ...\__init__.py:4:1: E402 module level import not at top of file

My environment

  • Windows 10
  • python 3.8

How I install flake8: poetry add flake8 which in fact run pip install <dependency> --no-deps for every flake8 dependency before installing the package itself.

That sounds like a regression :/

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on May 31, 2020, 13:06

hello @lewoudar,

that's unrelated to this issue, you can read more about it in #638

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @lewoudar on May 31, 2020, 13:16

Aww thanks for the reference, I will check that

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @tmewett on Nov 3, 2020, 08:59

Is it necessary to involve the location of the config file? With the full current working directory and the relative path of what is being scanned by flake8, is it possible to construct a full valid path for any file being scanned. This can then be matched against the configured path patterns.

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @asottile on Nov 3, 2020, 09:09

yes, the expectation of flake8 users is that paths in config are relative to it -- this is true for all options currently except per-file-ignores

the config is not always in the current working directory

@asottile
Copy link
Member Author

asottile commented Apr 3, 2021

In GitLab by @tmewett on Nov 3, 2020, 09:52

That is certainly the expectation of current flake8 users, but just fyi it contrasts with other tools which have file pattern config files. Anyway, I raised a separate issue #693 for this as it's not completely related.

rbpatt2019 added a commit to IMS-Bio2Core-Facility/BIC086 that referenced this issue Jun 4, 2021
Previous attempts to selectively disable this warning per-file failed,
as flake8 does not correctly normalise paths for `per-file-ignores`.
Until resolved, we disable this check as `assert` statements are
extremely unlikely out of tests files.

See PyCQA/flake8#693
@kaybee99

This comment was marked as off-topic.

gsakkis added a commit to single-cell-data/TileDB-SOMA that referenced this issue Jun 24, 2022
…flake8 per-file-ignore

Reason: per-file-ignores paths are treated relative to the current directory, not the config file.
This breaks the flake8 pre-commit hook that is invoked from the project root.
See PyCQA/flake8#693
gsakkis added a commit to single-cell-data/TileDB-SOMA that referenced this issue Jun 26, 2022
…flake8 per-file-ignore

Reason: per-file-ignores paths are treated relative to the current directory, not the config file.
This breaks the flake8 pre-commit hook that is invoked from the project root.
See PyCQA/flake8#693
gsakkis added a commit to single-cell-data/TileDB-SOMA that referenced this issue Jun 26, 2022
…flake8 per-file-ignore

Reason: per-file-ignores paths are treated relative to the current directory, not the config file.
This breaks the flake8 pre-commit hook that is invoked from the project root.
See PyCQA/flake8#693
johnkerl pushed a commit to single-cell-data/TileDB-SOMA that referenced this issue Jun 30, 2022
* Add pre-commit hooks

* Specify explicitly all exportable names from tiledbsc and remove the flake8 per-file-ignore

Reason: per-file-ignores paths are treated relative to the current directory, not the config file.
This breaks the flake8 pre-commit hook that is invoked from the project root.
See PyCQA/flake8#693

* Lint Python scripts outside of apis/python
@Guillem96

This comment was marked as off-topic.

@asottile
Copy link
Member Author

@Guillem96 OSS advice: github provides a thumbs up so you can participate without sending an email to everyone subscribed. it's also easy to check the status of an issue: is it open? yes. if you have doubts on the freshness of the issue it's very easy to just try it

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

No branches or pull requests

3 participants