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

Lint all files in a directory by expanding arguments #5682

Merged
merged 34 commits into from
Feb 6, 2022
Merged
Show file tree
Hide file tree
Changes from 29 commits
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
5 changes: 5 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ Release date: TBA
..
Put new features here and also in 'doc/whatsnew/2.13.rst'

* Add ``--recursive`` option to allow recursive discovery of all modules and packages in subtree. Running pylint with
``--recursive=y`` option will check all discovered ``.py`` files and packages found inside subtree of directory provided
as parameter to pylint.

Closes #352

* Fixed crash from ``arguments-differ`` and ``arguments-renamed`` when methods were
defined outside the top level of a class.
Expand Down
21 changes: 21 additions & 0 deletions doc/faq.rst
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,27 @@ For example::

Much probably. Read :ref:`ide-integration`

3.5 I need to run pylint over all modules and packages in my project directory.
-------------------------------------------------------------------------------

By default the ``pylint`` command only accepts a list of python modules and packages. Using a
directory which is not a package results in an error::

pylint mydir
************* Module mydir
mydir/__init__.py:1:0: F0010: error while code parsing: Unable to load file mydir/__init__.py:
[Errno 2] No such file or directory: 'mydir/__init__.py' (parse-error)

To execute pylint over all modules and packages under the directory, the ``--recursive=y`` option must
be provided. This option makes ``pylint`` attempt to discover all modules (files ending with ``.py`` extension)
and all packages (all directories containing a ``__init__.py`` file).
Those modules and packages are then analyzed::

pylint --recursive=y mydir

When ``--recursive=y`` option is used, modules and packages are also accepted as parameters::

pylint --recursive=y mydir mymodule mypackage

4. Message Control
==================
Expand Down
4 changes: 4 additions & 0 deletions doc/user_guide/run.rst
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ will work if ``directory`` is a python package (i.e. has an __init__.py
file or it is an implicit namespace package) or if "directory" is in the
python path.

By default, pylint will exits an error when one of the arguments is a directory which is not
matusvalo marked this conversation as resolved.
Show resolved Hide resolved
a python package. In order to run pylint over all modules and packages within the provided
subtree of a directory, the ``--recursive=y`` option must be provided.

For more details on this see the :ref:`faq`.

It is also possible to call Pylint from another Python program,
Expand Down
6 changes: 6 additions & 0 deletions doc/whatsnew/2.13.rst
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@ Extensions
Other Changes
=============

* Add ``--recursive`` option to allow recursive discovery of all modules and packages in subtree. Running pylint with
``--recursive=y`` option will check all discovered ``.py`` files and packages found inside subtree of directory provided
as parameter to pylint.

Closes #352
matusvalo marked this conversation as resolved.
Show resolved Hide resolved

* Fixed crash from ``arguments-differ`` and ``arguments-renamed`` when methods were
defined outside the top level of a class.

Expand Down
38 changes: 38 additions & 0 deletions pylint/lint/pylinter.py
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,15 @@ def make_options() -> Tuple[Tuple[str, OptionDict], ...]:
),
},
),
(
"recursive",
matusvalo marked this conversation as resolved.
Show resolved Hide resolved
{
"type": "yn",
"metavar": "<yn>",
"default": False,
"help": "Discover python modules and packages in the file system subtree.",
},
),
(
"py-version",
{
Expand Down Expand Up @@ -1005,6 +1014,33 @@ def initialize(self):
if not msg.may_be_emitted():
self._msgs_state[msg.msgid] = False

@staticmethod
def _discover_files(files_or_modules: Sequence[str]) -> Iterator[str]:
matusvalo marked this conversation as resolved.
Show resolved Hide resolved
"""Discover python modules and packages in subdirectory.

Returns iterator of paths to discovered modules and packages.
"""
for something in files_or_modules:
if os.path.isdir(something) and not os.path.isfile(
os.path.join(something, "__init__.py")
):
skip_subtrees: List[str] = []
for root, _, files in os.walk(something):
if any(root.startswith(s) for s in skip_subtrees):
# Skip subtree of already discovered package.
continue
if "__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 +1055,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

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

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))
matusvalo marked this conversation as resolved.
Show resolved Hide resolved
if self.config.from_stdin:
if len(files_or_modules) != 1:
raise exceptions.InvalidArgsError(
Expand Down
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
Empty file.
72 changes: 63 additions & 9 deletions tests/test_self.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,24 @@ def _configure_lc_ctype(lc_ctype: str) -> Iterator:
os.environ[lc_ctype_env] = original_lctype


@contextlib.contextmanager
Copy link
Member

Choose a reason for hiding this comment

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

This could probably even go in tests/conftest.py to be used everywhere ? I don't know how frequent it is. But we might want to do that in another MR, this one need to be merged πŸ˜„

def _test_sys_path() -> Generator[None, None, None]:
original_path = sys.path
try:
yield
finally:
sys.path = original_path


@contextlib.contextmanager
def _test_cwd() -> Generator[None, None, None]:
original_dir = os.getcwd()
try:
yield
finally:
os.chdir(original_dir)


class MultiReporter(BaseReporter):
def __init__(self, reporters: List[BaseReporter]) -> None:
# pylint: disable=super-init-not-called
Expand Down Expand Up @@ -810,14 +828,6 @@ def test_fail_on_edge_case(self, opts, out):

@staticmethod
def test_modify_sys_path() -> None:
@contextlib.contextmanager
def test_sys_path() -> Generator[None, None, None]:
original_path = sys.path
try:
yield
finally:
sys.path = original_path

@contextlib.contextmanager
def test_environ_pythonpath(
new_pythonpath: Optional[str],
Expand All @@ -837,7 +847,7 @@ def test_environ_pythonpath(
# Only delete PYTHONPATH if new_pythonpath wasn't None
del os.environ["PYTHONPATH"]

with test_sys_path(), patch("os.getcwd") as mock_getcwd:
with _test_sys_path(), patch("os.getcwd") as mock_getcwd:
cwd = "/tmp/pytest-of-root/pytest-0/test_do_not_import_files_from_0"
mock_getcwd.return_value = cwd
default_paths = [
Expand Down Expand Up @@ -1290,3 +1300,47 @@ def test_regex_paths_csv_validator() -> None:
with pytest.raises(SystemExit) as ex:
Run(["--ignore-paths", "test", join(HERE, "regrtest_data", "empty.py")])
assert ex.value.code == 0

def test_regression_recursive(self):
Copy link
Member

Choose a reason for hiding this comment

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

Tests are simple yet effective. Very nice πŸ‘

self._test_output(
[join(HERE, "regrtest_data", "directory", "subdirectory"), "--recursive=n"],
expected_output="No such file or directory",
)

def test_recursive(self):
self._runtest(
[join(HERE, "regrtest_data", "directory", "subdirectory"), "--recursive=y"],
matusvalo marked this conversation as resolved.
Show resolved Hide resolved
code=0,
)

def test_recursive_current_dir(self):
with _test_sys_path():
sys.path = [
path
for path in sys.path
if not os.path.basename(path) == "regrtest_data"
]
# pytest is including directory HERE/regrtest_data to sys.path which causes
# astroid believe that directory is a package.
with _test_cwd():
os.chdir(join(HERE, "regrtest_data", "directory"))
self._runtest(
[".", "--recursive=y"],
code=0,
)

def test_regression_recursive_current_dir(self):
with _test_sys_path():
# pytest is including directory HERE/regrtest_data to sys.path which causes
# astroid believe that directory is a package.
matusvalo marked this conversation as resolved.
Show resolved Hide resolved
sys.path = [
path
for path in sys.path
if not os.path.basename(path) == "regrtest_data"
]
with _test_cwd():
os.chdir(join(HERE, "regrtest_data", "directory"))
self._test_output(
["."],
expected_output="No such file or directory",
)