Skip to content

Lint all files in a directory by expanding arguments #5682

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.

Sign up for GitHub

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

Merged
merged 34 commits into from
Feb 6, 2022
Merged

Lint all files in a directory by expanding arguments #5682

Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
e44977d
Added --recursive option
matusvalo Jan 14, 2022
9b23124
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jan 14, 2022
20ad408
Added tests for --recursive option
matusvalo Jan 15, 2022
2aceafb
Added comments, types and make linters happy
matusvalo Jan 15, 2022
404bb9b
Updated Changelog and whatsnew
matusvalo Jan 15, 2022
44f1919
Improved help message
matusvalo Jan 15, 2022
b42df9c
Merge branch 'main' into discover_modules2
matusvalo Jan 15, 2022
3ebb1d7
Mention --recursive option in faq and user guide
matusvalo Jan 16, 2022
6520a4e
Merge branch 'discover_modules2' of https://github.com/matusvalo/pyli…
matusvalo Jan 16, 2022
8741104
Fix typos
matusvalo Jan 17, 2022
91fe106
Use --recursive=y/n instead of just --recursive
matusvalo Jan 18, 2022
6623eb1
Enable recursive mode when pylint is executed over current directory
matusvalo Jan 19, 2022
bc45736
Revert "Enable recursive mode when pylint is executed over current di…
matusvalo Jan 21, 2022
8ae1438
Update doc/faq.rst
matusvalo Jan 26, 2022
756acc5
Update pylint/lint/pylinter.py
matusvalo Jan 26, 2022
e83f9fc
Update doc/user_guide/run.rst
matusvalo Jan 26, 2022
ca13575
Update doc/user_guide/run.rst
matusvalo Jan 26, 2022
c9e571e
Update doc/faq.rst
matusvalo Jan 26, 2022
68529b5
Update doc/faq.rst
matusvalo Jan 26, 2022
b5e5bb6
Update doc/faq.rst
matusvalo Jan 26, 2022
6199d4e
Update doc/faq.rst
matusvalo Jan 26, 2022
59480dc
Update doc/user_guide/run.rst
matusvalo Jan 26, 2022
f47c9b8
Added unittests for current directory
matusvalo Jan 26, 2022
03a0890
Merge branch 'discover_modules2' of https://github.com/matusvalo/pyli…
matusvalo Jan 26, 2022
1de1999
Apply suggestions from code review
matusvalo Jan 26, 2022
a86cf14
Make changelog entry same as entry in doc/whatsnew/2.13.rst
matusvalo Jan 26, 2022
3d96598
correctly revert chdir in unittests
matusvalo Jan 27, 2022
20dcce4
Fix failing test_regression_recursive_current_dir
matusvalo Jan 29, 2022
1aa4fdc
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jan 29, 2022
154f7d9
Cosmetics
matusvalo Jan 29, 2022
0864585
Merge branch 'main' into discover_modules2
Pierre-Sassoulas Feb 2, 2022
8bf62cb
Merge remote-tracking branch 'upstream/main' into discover_modules2
matusvalo Feb 3, 2022
5735618
Apply suggestions from code review
matusvalo Feb 4, 2022
ca79dae
Apply suggestions from code review
matusvalo Feb 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions pylint/lint/pylinter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,13 @@ def make_options() -> Tuple[Tuple[str, OptionDict], ...]:
),
},
),
(
"recursive",
{
"action": "store_true",
"help": ("Discover python files in file system subtree."),
},
),
(
"py-version",
{
Expand Down Expand Up @@ -1005,6 +1012,22 @@ def initialize(self):
if not msg.may_be_emitted():
self._msgs_state[msg.msgid] = False

def _discover_files(self, files_or_modules):
for something in files_or_modules:
if os.path.isdir(something) and not os.path.isfile(os.path.join(something, '__init__.py')):
skip_subtrees = []
for root, dirs, files in os.walk(something):
if any(root.startswith(s) for s in skip_subtrees):
# Skip subtree of already discovered package
continue
elif '__init__.py' in files:
skip_subtrees.append(root)
yield root
else:
yield from (os.path.join(root, file) for file in files if file.endswith('.py'))
else:
yield something

def check(self, files_or_modules: Union[Sequence[str], str]) -> None:
"""main checking entry: check a list of files or modules from their name.

Expand All @@ -1019,6 +1042,8 @@ def check(self, files_or_modules: Union[Sequence[str], str]) -> None:
DeprecationWarning,
)
files_or_modules = (files_or_modules,) # type: ignore[assignment]
if self.config.recursive:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to happen before in the initialization but should we make recursive true, if pylint is called without directory (pylint --rc-file=...), or with . (pylint . --rc-file=...) and if __init__.pydoes not exists in the current directory ? I think the intent to use recursive is clear for those two as it would only work if there was an __init__.py present. So we could fix it right now (before releasing pylint 3.0). On the other hand modifying behavior based on other argument than the one controlling the behavior feels wrong. @DanielNoord, thought ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to happen before in the initialization but should we make recursive true, if pylint is called without directory (pylint --rc-file=...), or with . (pylint . --rc-file=...) and if __init__.pydoes not exists in the current directory ?

I don't think pylint --rc-file=... should be allowed. It should either be pylint . --rc-file=... or with another directory.
We could discuss allowing pylint --rc-file=... if we add support for a files option in rc-file, but running pylint without any clear intent on what directory to lint wouldn't be good imo. How would we display the help message then?

I think the intent to use recursive is clear for those two as it would only work if there was an __init__.py present. So we could fix it right now (before releasing pylint 3.0). On the other hand modifying behavior based on other argument than the one controlling the behavior feels wrong. @DanielNoord, thought ?

I agree that recursive=y should be set when called with . as directory. The same probably goes for missing __init__.py. However, those would be breaking changes and I wonder how much depends on this currently not being supported. I'd be hesitant to change this in a minor version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think pylint --rc-file=... should be allowed.

pytest does this I think it's a nice default value. It was asked here and has currently 12 upvotes

How would we display the help message then?

pylint --help, or pylint -h, which I think is the expectation for CLI tool. (also pylint --long-help because we're special 😄)

I agree that recursive=y should be set when called with . as directory. The same probably goes for missing init.py.

I meant both . and no __init__.py in the current directory at the same time. In this configuration the current result would be a fatal and nothing working so I guess it would not be a real breaking change (although there's probably someone somewhere who check for pylint exiting with a fatal instead of checking for an __init__.py 😄 ).

You made me realize that in fact we can make recursive true when there is no __init__.py in any directory we lint and it does not matter if it's . the argument that it was not working at all before is still true. But changing the recusrive option for . would be a breaking change as pylint could be working before if there was an __init__.py in the top level directory, but would start to lint new files unexpectedly.

Copy link
Collaborator

@DanielNoord DanielNoord Jan 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pytest does this I think it's a nice default value. It was asked here and has currently 12 upvotes

Mypy also allows this but only when you set files in your config. I really think we shouldn't allow this without setting files first.

pylint --help, or pylint -h, which I think is the expectation for CLI tool. (also pylint --long-help because we're special 😄)

My expectation is that just running the command also gives a short help message. I know so many CLI tool that do this.

Edit: Let me rephrase, I know that pytest doesn't do this but it has done so for as long as I can remember I think. Changing the behaviour of running pylint without any arguments is a real breaking change which I would argue against. If we would support files and recursive people can just set those and then run pylint without args if they want to.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas Jan 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My expectation is that just running the command also gives a short help message. I know so many CLI tool that do this.

Yes, this is what happen when the tool can't work without providing arguments. For example ls or pwd, pytest or flake8 can run without args so they do. mv or scp can't, so they display the help (or something telling you how to display the help in the case of mv):

$scp
usage: scp [-346BCpqrTv] [-c cipher] [-F ssh_config] [-i identity_file]
            [-J destination] [-l limit] [-o ssh_option] [-P port]
            [-S program] source ... target

Imo, pylint should work without any argument (like flake8). But I wonder why mypy do not though. Do you know the rational behind it ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 1, no, it should show the help like before, we did not reach a consensus on this with @DanielNoord and I'm going to create an issue to settle it by popular vote, once we merge this one. (By the way, what do you think of defaulting to "." ? :) )

For 2. yes exactly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for myself but perhaps helpful for others. I think for now this is the summary:
For 2.x:
When in spam without __init__.py:
pylint . > Recursive
pylint > Exit without doing anything.
pylint ../spam > ??? (Recursive I guess)?

When in spam with __init__.py:
pylint . > not recursive
pylint > Exit without doing anything.
pylint ../spam > ??? (Not recursive I guess)?

When in parent without __init__.py of spam without __init__.py:
pylint . > Recursive, finds spam
pylint spam > Recursive
pylint > Exit without doing anything.
pylint ../. > ??? (Recursive I guess)?

When in parent without __init__.py of spam with __init__.py:
pylint . > Recursive, finds spam
pylint spam > Not recursive
pylint > Exit without doing anything.
pylint ../. > ??? (Recursive I guess)?

For 3.0. In bold is those that changed compared to 2.x:
When in spam without __init__.py:
pylint . > Recursive
pylint > TBD.
pylint ../spam > Recursive

When in spam with __init__.py:
pylint . > Recursive
pylint > TBD.
pylint ../spam > Recursive

When in parent without __init__.py of spam without __init__.py:
pylint . > Recursive, finds spam
pylint spam > Recursive
pylint > TBD.
pylint ../. > Recursive

When in parent without __init__.py of spam with __init__.py:
pylint . > Recursive, finds spam
pylint spam > Recursive
pylint > TBD
pylint ../. > Recursive

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

??? (Recursive I guess)?

I would say those should indeed be recursive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last commit implements default recursive mode when . directory is passed to pylint.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave this comment as unresolved as it is quite important for future people looking back at this PR and it might get easily lost.

files_or_modules = tuple(self._discover_files(files_or_modules))
if self.config.from_stdin:
if len(files_or_modules) != 1:
raise exceptions.InvalidArgsError(
Expand Down