Add ignore-paths to match against the full path
#3266
Conversation
bernardn
force-pushed
the
issue/2541
branch
4 times, most recently
from
d574e15
to
2afb4d6
Compare
There was a problem hiding this comment.
Hey @bernardn Thank you for sending the PR over! Overall it looks great, I just have a minor concern over the .match() behaviour which seems backwards incompatible to me.
pylint/utils/utils.py
Outdated
| :param list black_list_re: A collection of regex patterns to match against. | ||
| Successful matches are blacklisted. | ||
|
|
||
| :returns: `True` if the basename is blacklisted, `False` otherwise. | ||
| :rtype: bool | ||
| """ | ||
| for file_pattern in black_list_re: | ||
| if file_pattern.match(base_name): | ||
| if file_pattern.match(path): |
There was a problem hiding this comment.
I believe this is a backwards incompatible change. Users that currently rely on ignore-patterns will suddenly see warnings from files they previously ignored. Given a full match might be too much, we might use .search here instead.
We should add a test for that to make sure what patterns that assumed this works on the basename will still filter out the corresponding files.
There was a problem hiding this comment.
Hi @PCManticore Thank you for your constructive comment. .search would indeed decrease the number of cases where backward-incompatibility occurs, but won't completely eliminate them, particularly when the tree structure presents repeating patterns, e.g., with ignore-patterns=foo:
whatever/foo/bar.py
whatever/foo/foo-a.py
whatever/foo/foo-b.py
The current implementation would lint bar.py, while this PR with .match or .search would ignore it
I'm afraid that to be fully backward compatible without anyone having to rewrite its ignore-patterns configuration directive, we'd have to implement a new one, such as ignore-path-patterns, what I can do.
What do you think ?
There was a problem hiding this comment.
Hello, kind reminder on this old PR.
Personally I find new separate option safer and more flexible.
There was a problem hiding this comment.
I can update it, but would prefer to have @PCManticore thoughts before doing it
There was a problem hiding this comment.
@bernardn Sorry for letting this fall through the cracks. Here are my thoughts:
- I think we should be backwards compatible to mess as little with users configuration as possible. Let's go with a new
ignore-pathsoption, which will support regexes. - This will bring the choice for ignoring files or directories to three:
ignore-paths,ignoreandignore-patterns. Once we have the first, we can start deprecating the other two, so we could remove them in pylint 3.0. We'll emit deprecation warnings when detecting their usage. - Let's throw an error if these three are used together. e.g. detect if
ignore-pathsis used withignore-patternsand simply throw a new error frompylint.exceptionsthat this behaviour is not supported.
Let me know if that makes sense to you as well.
There was a problem hiding this comment.
@PCManticore This makes perfect sense to me as well, thank you! I'm going to implement this in the upcoming days.
|
@bernardn Could you rebase your PR to resolve the conflicts? |
bernardn
force-pushed
the
issue/2541
branch
4 times, most recently
from
683c042
to
95a8b51
Compare
List of regex matching against the full path Close pylint-dev#2541
bernardn
changed the title
ignore-patterns match against the full path instead of basenameignore-paths to match against the full path
|
@PCManticore did you approve this PR? |
|
When is this PR getting merged to master? |
|
@sanjayatn I'll do a review of this in the coming weeks and solve the conflicts |
|
any progress ? @brycepg |
Pierre-Sassoulas
added
Help wanted 🙏
|
Closed in favor of #4516 |
Steps
doc/whatsnew/<current release.rst>.Description
Add
ignore-pathsconfiguration directive. Checks against full file path.Type of Changes
Related Issue
Closes #2541