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

Do not remove preceding ./ from ignore-paths option input #5491

Open
matejcik opened this issue Dec 7, 2021 · 12 comments
Open

Do not remove preceding ./ from ignore-paths option input #5491

matejcik opened this issue Dec 7, 2021 · 12 comments
Labels
Bug 🪲 Configuration Related to configuration Needs PR This issue is accepted, sufficiently specified and now needs an implementation

Comments

@matejcik
Copy link

matejcik commented Dec 7, 2021

Bug description

When run as pylint test.py --ignore-paths=./test.py, Pylint (correctly?) ignores the file and produces no output.

When run as pylint ./test.py --ignore-paths=./test.py, the file test.py is NOT ignored and it is instead checked.

Configuration

Empty configuration.

Command used

pylint ./test.py --ignore-paths=./test.py

Pylint output

************* Module test
test.py:1:0: C0114: Missing module docstring (missing-module-docstring)
...
(contents of file are not relevant)

Expected behavior

(no output)

Pylint version

pylint 2.13.0-dev0
astroid 2.9.0
Python 3.8.10 (default, Sep 28 2021, 16:10:42) 
[GCC 9.3.0]

I'm seeing the same behavior since version 2.12

OS / Environment

No response

Additional dependencies

No response

@matejcik matejcik added Bug 🪲 Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Dec 7, 2021
@Pierre-Sassoulas Pierre-Sassoulas removed the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Dec 7, 2021
@DanielNoord
Copy link
Collaborator

Please refer to the documentation for this setting here: https://pylint.pycqa.org/en/latest/technical_reference/features.html

ignore-paths is regular expression pattern that matches to the path of the file. The path will likely be something like ~/YourLocalDir/test.py.
Thus a relative ignore pattern would be .*/test.py. Note the asterisk after the dot. This should solve the issue you're reporting. If not, please let me know and I can reopen the issue!

@DanielNoord DanielNoord added Question Configuration Related to configuration and removed Bug 🪲 labels Dec 7, 2021
@matejcik
Copy link
Author

matejcik commented Dec 7, 2021

The documentation does not specify what gets passed in to the regexp. Is it always an absolute path?
(if yes, that's unfortunate, as it makes it impossible to match from the project root -- how would I specify ignore pattern that skips <root>/test.py but does not skip <root>/src/somepkg/test.py?)

It would be best, IMHO, if the path was always relative to the project root (location of setup.cfg, or cwd of the pylint invocation for a simpler option), that way I could specify the pattern as ^test.py and know the right thing is being matched.

Regardless, why is there a difference in behavior between passing pylint test.py and pylint ./test.py ?

@Pierre-Sassoulas
Copy link
Member

In a regex . matches any character (but there have to be a character). So it match a/test.py but not test.py.

@matejcik
Copy link
Author

matejcik commented Dec 7, 2021

I know.
Why does it not match the literal . when I pass in the file as ./test.py

@matejcik
Copy link
Author

matejcik commented Dec 7, 2021

Let me rephrase my question:
what is the path string that gets matched against the --ignore-paths regexp?

My file test.py exists at absolute path /home/matejcik/a/bbb/test.py
When I run pylint from the /home/matejcik/a/bbb directory, I can do the following:

$ pylint --ignore-paths=./test.py test.py
# no output -- the file is ignored

In this case, I would expect the path string to either be
(a) the full path, that is, /home/matejcik/a/bbb/test.py, which does NOT match the regexp ./test.py
(b) the argument-as-passed in, that is, test.py, which also does NOT match the regexp,
(c) something strange inbetween, like, say, ~/a/bbb/test.py, which also does NOT match the regexp.
(d) path relative to the CWD, so ./test.py, which DOES match the regexp.

However, the issue at hand is:

$ pylint --ignore-paths=./test.py ./test.py
# some output - the file is checked

How is the argument-as-passed-in processed so that it suddenly doesn't match the same regexp?

I can reproduce the same problem one directory up:

$ pylint --ignore-paths=./bbb/test.py bbb/test.py
# no output - file is ignored
$ pylint --ignore-paths=./bbb/test.py ./bbb/test.py
# some output - file is checked

@DanielNoord
Copy link
Collaborator

The documentation does not specify what gets passed in to the regexp. Is it always an absolute path?

No it matches against the path provided in the command. So if doing pylint test.py the path will be path.py.

(if yes, that's unfortunate, as it makes it impossible to match from the project root -- how would I specify ignore pattern that skips <root>/test.py but does not skip <root>/src/somepkg/test.py?)

<root>/test.py would only skip <root>/test.py. Similarly test.py would also only skip it as src/somepkg/test.py does not match test.py.

It would be best, IMHO, if the path was always relative to the project root (location of setup.cfg, or cwd of the pylint invocation for a simpler option), that way I could specify the pattern as ^test.py and know the right thing is being matched.

I think ^test.py should work as well.

We remove the ./ from the ignore pattern as we normalise the pattern to a Posix and Windows path with pathlib.PureWindowsPath. This removes the preceding ./.

Regardless, why is there a difference in behavior between passing pylint test.py and pylint ./test.py ?

This is because of the normalisation and perhaps something we should take into account. I'm wondering if we can add ./ to every ignore pattern and still be save..

@DanielNoord
Copy link
Collaborator

DanielNoord commented Dec 7, 2021

Let me rephrase my question: what is the path string that gets matched against the --ignore-paths regexp?

My file test.py exists at absolute path /home/matejcik/a/bbb/test.py When I run pylint from the /home/matejcik/a/bbb directory, I can do the following:

$ pylint --ignore-paths=./test.py test.py
# no output -- the file is ignored

In this case, I would expect the path string to either be (a) the full path, that is, /home/matejcik/a/bbb/test.py, which does NOT match the regexp ./test.py (b) the argument-as-passed in, that is, test.py, which also does NOT match the regexp, (c) something strange inbetween, like, say, ~/a/bbb/test.py, which also does NOT match the regexp. (d) path relative to the CWD, so ./test.py, which DOES match the regexp.

However, the issue at hand is:

$ pylint --ignore-paths=./test.py ./test.py
# some output - the file is checked

How is the argument-as-passed-in processed so that it suddenly doesn't match the same regexp?

I can reproduce the same problem one directory up:

$ pylint --ignore-paths=./bbb/test.py bbb/test.py
# no output - file is ignored
$ pylint --ignore-paths=./bbb/test.py ./bbb/test.py
# some output - file is checked

This is all related to my last point in the previous comment which I just posted after you posted this:
We remove the preceding ./ from the ignore pattern as we use pathlib.PureWindowsPath. Therefore in the first example we're actually matching test.py against test.py and thus ignore, while the second example matches test.py against ./test.py and thus check.

This is indeed something that should be fixed! I'll reopen and rename the issue!

For future reference:
The pathlib documentation mentions this behaviour of collapsing dots here:
https://docs.python.org/3/library/pathlib.html#pathlib.PurePath

@DanielNoord DanielNoord reopened this Dec 7, 2021
@DanielNoord DanielNoord changed the title ignore-paths broken when passing a relative path Do not remove preceding ./ from ignore-paths option input Dec 7, 2021
@huxuan
Copy link

huxuan commented Mar 28, 2022

Hi @DanielNoord, so any suggestions to fix this issue? I think maybe we could use os.path.relpath() to decide whether we should check? Since os.path.relpath("./test.py", ".") == "test.py".

If OK, I can try it fix it as my first pull request. :-)

@DanielNoord
Copy link
Collaborator

The changes that cause this were introduced in #5201. I think the tests that were added to tests/unittest_config.py in that PR are fairly robust and might help you fix this and see if any solution is "good".

I honestly don't know what the best approach is here (and must warn that it might be a difficult issue for a first PR to a project).
I think there are two approaches:

  1. Try and see if we can remove the ./ from the path before the normalisation with patlib. However, this might be difficult as we would then need to find a way to add it back again while handling the different path separators.
  2. A second approach I was thinking of is: why don't we normalise the path files that we are going to check? I have not thought about the full implications of this, but I don't immediately see why we don't normalise the "files" argument ./test.py to test.py before checking it against patterns. This probably takes place somewhere in the Run class so that would be my first point of investigation.

Approach 2 does risk being non backwards compatible if people rely on us not normalising so that might be an issue...

@huxuan
Copy link

huxuan commented Mar 28, 2022

The changes that cause this were introduced in #5201. I think the tests that were added to tests/unittest_config.py in that PR are fairly robust and might help you fix this and see if any solution is "good".

I honestly don't know what the best approach is here (and must warn that it might be a difficult issue for a first PR to a project). I think there are two approaches:

  1. Try and see if we can remove the ./ from the path before the normalisation with patlib. However, this might be difficult as we would then need to find a way to add it back again while handling the different path separators.
  2. A second approach I was thinking of is: why don't we normalise the path files that we are going to check? I have not thought about the full implications of this, but I don't immediately see why we don't normalise the "files" argument ./test.py to test.py before checking it against patterns. This probably takes place somewhere in the Run class so that would be my first point of investigation.

Approach 2 does risk being non backwards compatible if people rely on us not normalising so that might be an issue...

Thanks for the detailed explanation. I will further look into it. Actually, I was blocked by the issue. As the release of v2.13.0, I try to use pylint . for all the projects, but it does not work for pre-commit, as pre-commit does not have ./ preceding. So currently I have to use (./)?test.py for hack.

@Pierre-Sassoulas Pierre-Sassoulas added the Needs PR This issue is accepted, sufficiently specified and now needs an implementation label Jul 13, 2022
@Pierre-Sassoulas
Copy link
Member

Hi @huxuan, are you still working on this ? (Not any obligation it's only so potential contributor know if they can do this issue without stepping on your toes :) )

@huxuan
Copy link

huxuan commented Jul 13, 2022

Hi @huxuan, are you still working on this ? (Not any obligation it's only so potential contributor know if they can do this issue without stepping on your toes :) )

Hi @Pierre-Sassoulas, I am not working on it. Thanks for your consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 Configuration Related to configuration Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

No branches or pull requests

4 participants