Lint all files in a directory by expanding arguments #5682

matusvalo · 2022-01-14T22:53:47Z

Type of Changes

	Type
	🐛 Bug fix
✓	✨ New feature
	🔨 Refactoring
	📜 Docs

Description

This PR is alternative to #5676 trying to resolve sys.path adjustment issue mentioned in #352 (comment).

The main rationale of this PR is to introduce recursive discovery of python modules/packages transparently to rest of pylint components. This is done by
expanding arguments containing directories with all modules and packages inside this directory. This approach should be safe with #352 (comment) since this change is totally transparent to sys.path adjustments in pylint. Hence for following subtree:

- test
     `+- a.py
      +- b.py
      +- c
          `+- __init__.py
           +- d.py

Executing command pylint --recursive test is equal to executing command pylint test/a.py test/b.py test/c.

Closes #352

for more information, see https://pre-commit.ci

pylint/lint/pylinter.py

matusvalo · 2022-01-14T23:01:02Z

Marking as draft, since still missing polishing, tests and documentation. I would like to ask just for review of approach.

coveralls · 2022-01-14T23:11:10Z

Pull Request Test Coverage Report for Build 1799934875

14 of 15 (93.33%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 93.935%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/lint/pylinter.py	14	15	93.33%

Totals
Change from base Build 1784689763:	0.02%
Covered Lines:	14854
Relevant Lines:	15813

💛 - Coveralls

Pierre-Sassoulas

Yeah, I think this is pretty clever. In fact this is what was done currently (in bash or pre-commit by each users) when launching pylint with a long list of files, so imo this is pretty safe too. We're probably missing some optimization of doing the sys path manipulation less internally but the alternative is a very complex change and waiting even longer for what has been the most wanted feature of the project for years. Goob job !

DanielNoord · 2022-01-15T08:53:48Z

I think this is also pretty clever!

I wonder if we could make it so that pylint . is equal to pylint . -r. I think most users would want us to do that as well. Perhaps a bit out of the scope for this PR, but I think if we want to go ahead and do that we should include a PR that does so in the same release so it works as intended from the start.

Pierre-Sassoulas · 2022-01-15T08:57:01Z

I wonder if we could make it so that pylint . is equal to pylint . -r.

pylint should also be equal to pylint . -r as well. I also think -r should become the default option in 3.0.

DanielNoord · 2022-01-15T08:59:32Z

I wonder if we could make it so that pylint . is equal to pylint . -r.

pylint should also be equal to pylint . -r as well. I also think -r should become the default option in 3.0.

Agreed! But I was wondering if for 2.x we can already make pylint . work like -r was provided. Although I wonder how many projects use pylint . already... Hm perhaps we shouldn't do this and just add it to the 3.0 list.

Pierre-Sassoulas · 2022-01-15T09:04:11Z

already make pylint . work like -r was provided.

The intent is pretty clear when doing that imo, so I'd say it's safe to put in 2.x.

In fact my first instinct was to also make recursive the default in 2.x. But there's probably projects somewhere that use this "non feature" to exclude files from the linting,

matusvalo · 2022-01-15T09:44:19Z

Thank you for your feedback. Let me finish the PR. I will close the previous one.

matusvalo · 2022-01-15T21:56:00Z

Test added, code cleaned up, added type annotations and docstrings.

I have 2 questions:

About --recursive option. Right with no option there is old behaviour and later we should have --recursive option as default. But how will user then force old behaviour after the switch? New option will be added or we want to use --recursive=y/n option (in this case we won't need new option after)
Should this be documented in pylint userguide (running pylint chapter)? It is useful to have there but soon it will be default if I understand that correctly.

Note: I used only --recursive option. -r option is already used for other purposes (for reports)

Pierre-Sassoulas · 2022-01-15T22:42:05Z

About --recursive option. Right with no option there is old behaviour and later we should have --recursive option as default. But how will user then force old behaviour after the switch? New option will be added or we want to use --recursive=y/n option (in this case we won't need new option after)

If this was a new project, I would add a --no-recursive option, but in pylint the --recursive=y/n is clearly the thing user would expect. I don't think we'll break this convention even if we switch to argparse and are forced to do some breaking changes for 3.0, what do you think @DanielNoord ?

DanielNoord · 2022-01-16T10:04:47Z

About --recursive option. Right with no option there is old behaviour and later we should have --recursive option as default. But how will user then force old behaviour after the switch? New option will be added or we want to use --recursive=y/n option (in this case we won't need new option after)

If this was a new project, I would add a --no-recursive option, but in pylint the --recursive=y/n is clearly the thing user would expect. I don't think we'll break this convention even if we switch to argparse and are forced to do some breaking changes for 3.0, what do you think @DanielNoord ?

Would using --recursive=y/n allow equating --recursive with --recursive=y? I think being able to just use --recursive would be a highly request feature otherwise. Perhaps something with making all of these go to the same dest and making the options mutually exclusive? Like you did for pydocstringformatter.

Should this be documented in pylint userguide (running pylint chapter)? It is useful to have there but soon it will be default if I understand that correctly.

Yes, I would appreciate some documentation about it. If we make this default we can just update the documentation to reflect this!

… into discover_modules2

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

matusvalo · 2022-01-26T19:26:17Z

Tests are failing for TestRunTC.test_regression_recursive_current_dir but it is working on my local computer.

EDIT: it seems that the TC fails only when it is executed by command pytest tests. When file is executed directly pytest tests/test_self.py, the test is passing.

DanielNoord · 2022-01-29T16:44:04Z

@matusvalo Does this pass for you locally?

for more information, see https://pre-commit.ci

Pierre-Sassoulas · 2022-01-29T18:50:52Z

tests/test_self.py

@@ -100,6 +100,24 @@ def _configure_lc_ctype(lc_ctype: str) -> Iterator:
            os.environ[lc_ctype_env] = original_lctype


+@contextlib.contextmanager


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 😄

matusvalo · 2022-01-29T19:11:53Z

@matusvalo Does this pass for you locally?

Yes and no. the tests are failing only when executed by pytest tests as mentioned before. When you execute the file directly (pytest tests/test_self.py), it is working fine.

The reason is because pytest in case of executing tests via command pytest tests is adding to the sys.path also /Users/matus/dev/pylint/tests/regrtest_data path. Having this path causes that here:
https://github.com/PyCQA/pylint/blob/40ef1bebe9ee47ef0beef9b3ec53628d954710fa/pylint/lint/expand_modules.py#L65-L70
exception is not raised and modname is used not from exception handler but directly from return value of function. This is causing that pylint is not raising error for TestRunTC.test_regression_recursive_current_dir and hence failing the unittest.

But as said before only in case of running pytest tests. I have pushed the possible fix in last commit.

Note: I am not sure whether the behaviour causing failing tests is expected or it is bug in Astroid. But I think it is not connected to this PR.

matusvalo · 2022-02-03T17:09:47Z

I have merged main to fix conflict. @DanielNoord is there anything pending in this PR?

DanielNoord

Sorry @matusvalo, was swarmed at work and didn't really have the time to look into this.

One final question (and some spelling fixes). 😄

doc/user_guide/run.rst

pylint/lint/pylinter.py

tests/test_self.py

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

matusvalo · 2022-02-04T19:14:10Z

All suggestions resolved. Only point is just Iterator vs. Generator question. Let me know about it.

jacobtylerwalls

Daniel spotted these errors but was working just a bit quickly when suggesting the fix :-)

tests/test_self.py

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>

matusvalo · 2022-02-05T16:28:50Z

Thank you @jacobtylerwalls !

Pierre-Sassoulas · 2022-02-06T14:11:45Z

This is a huge improvement to pylint, thank you @matusvalo !

Refs: pylint-dev/pylint#352 pylint-dev/pylint#5682 Reverts: 2f5de90 4424ea1

matusvalo and others added 2 commits January 14, 2022 23:35

Added --recursive option

e44977d

[pre-commit.ci] auto fixes from pre-commit.com hooks

9b23124

for more information, see https://pre-commit.ci

matusvalo commented Jan 14, 2022

View reviewed changes

pylint/lint/pylinter.py Show resolved Hide resolved

matusvalo marked this pull request as draft January 14, 2022 23:00

Pierre-Sassoulas reviewed Jan 15, 2022

View reviewed changes

Pierre-Sassoulas mentioned this pull request Jan 15, 2022

Add option to allow linting over all files #4217

Closed

3 tasks

matusvalo mentioned this pull request Jan 15, 2022

Lint all files in a directory #5676

Closed

Added tests for --recursive option

20ad408

matusvalo force-pushed the discover_modules2 branch from 462b0e3 to 20ad408 Compare January 15, 2022 21:06

Added comments, types and make linters happy

2aceafb

matusvalo force-pushed the discover_modules2 branch from 6c3e3c8 to 2aceafb Compare January 15, 2022 21:30

Pierre-Sassoulas added Enhancement ✨ Improvement to a component Command line Related to command line interface labels Jan 15, 2022

Pierre-Sassoulas added this to the 2.13.0 milestone Jan 15, 2022

matusvalo and others added 3 commits January 15, 2022 22:44

Updated Changelog and whatsnew

404bb9b

Improved help message

44f1919

Merge branch 'main' into discover_modules2

b42df9c

matusvalo marked this pull request as ready for review January 15, 2022 21:50

matusvalo requested a review from Pierre-Sassoulas January 15, 2022 22:00

Mention --recursive option in faq and user guide

3ebb1d7

Merge branch 'discover_modules2' of https://github.com/matusvalo/pylint…

03a0890

… into discover_modules2

matusvalo requested a review from DanielNoord January 26, 2022 16:11

matusvalo and others added 2 commits January 26, 2022 19:00

Apply suggestions from code review

1de1999

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

Make changelog entry same as entry in doc/whatsnew/2.13.rst

a86cf14

correctly revert chdir in unittests

3d96598

matusvalo and others added 2 commits January 29, 2022 19:40

Fix failing test_regression_recursive_current_dir

20dcce4

[pre-commit.ci] auto fixes from pre-commit.com hooks

1aa4fdc

for more information, see https://pre-commit.ci

Pierre-Sassoulas reviewed Jan 29, 2022

View reviewed changes

matusvalo and others added 3 commits January 29, 2022 20:40

Cosmetics

154f7d9

Merge branch 'main' into discover_modules2

0864585

Merge remote-tracking branch 'upstream/main' into discover_modules2

8bf62cb

DanielNoord requested changes Feb 3, 2022

View reviewed changes

doc/user_guide/run.rst Outdated Show resolved Hide resolved

pylint/lint/pylinter.py Show resolved Hide resolved

tests/test_self.py Outdated Show resolved Hide resolved

tests/test_self.py Outdated Show resolved Hide resolved

Apply suggestions from code review

5735618

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>

matusvalo requested a review from DanielNoord February 4, 2022 19:13

jacobtylerwalls reviewed Feb 5, 2022

View reviewed changes

tests/test_self.py Outdated Show resolved Hide resolved

tests/test_self.py Outdated Show resolved Hide resolved

Apply suggestions from code review

ca79dae

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>

DanielNoord approved these changes Feb 5, 2022

View reviewed changes

Pierre-Sassoulas merged commit 2c6eadc into pylint-dev:main Feb 6, 2022

matusvalo deleted the discover_modules2 branch February 6, 2022 18:36

jstriebel mentioned this pull request Feb 9, 2022

upgrade dev-dependencies scalableminds/webknossos-libs#590

Closed

kdeldycke added a commit to kdeldycke/workflows that referenced this pull request Mar 28, 2022

Let Pylint discover Python files and modules to lint.

7821ce2

Refs: pylint-dev/pylint#352 pylint-dev/pylint#5682 Reverts: 2f5de90 4424ea1

Pierre-Sassoulas mentioned this pull request Jan 31, 2023

PEP420 cyclic-import false positive #7959

Closed

GrimzEcho mentioned this pull request Oct 24, 2023

--recursive option does not find .py files in sub-directories of packages #9187

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint all files in a directory by expanding arguments #5682

Lint all files in a directory by expanding arguments #5682

matusvalo commented Jan 14, 2022

matusvalo commented Jan 14, 2022

coveralls commented Jan 14, 2022 •

edited

Loading

Pierre-Sassoulas left a comment

DanielNoord commented Jan 15, 2022

Pierre-Sassoulas commented Jan 15, 2022

DanielNoord commented Jan 15, 2022

Pierre-Sassoulas commented Jan 15, 2022

matusvalo commented Jan 15, 2022

matusvalo commented Jan 15, 2022 •

edited

Loading

Pierre-Sassoulas commented Jan 15, 2022

DanielNoord commented Jan 16, 2022

matusvalo commented Jan 26, 2022 •

edited

Loading

DanielNoord commented Jan 29, 2022

Pierre-Sassoulas Jan 29, 2022

matusvalo commented Jan 29, 2022 •

edited

Loading

matusvalo commented Feb 3, 2022

DanielNoord left a comment

matusvalo commented Feb 4, 2022

jacobtylerwalls left a comment

matusvalo commented Feb 5, 2022

Pierre-Sassoulas commented Feb 6, 2022

		@@ -100,6 +100,24 @@ def _configure_lc_ctype(lc_ctype: str) -> Iterator:
		os.environ[lc_ctype_env] = original_lctype


		@contextlib.contextmanager

Lint all files in a directory by expanding arguments #5682

Lint all files in a directory by expanding arguments #5682

Conversation

matusvalo commented Jan 14, 2022

Type of Changes

Description

matusvalo commented Jan 14, 2022

coveralls commented Jan 14, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1799934875

💛 - Coveralls

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord commented Jan 15, 2022

Pierre-Sassoulas commented Jan 15, 2022

DanielNoord commented Jan 15, 2022

Pierre-Sassoulas commented Jan 15, 2022

matusvalo commented Jan 15, 2022

matusvalo commented Jan 15, 2022 • edited Loading

Pierre-Sassoulas commented Jan 15, 2022

DanielNoord commented Jan 16, 2022

matusvalo commented Jan 26, 2022 • edited Loading

DanielNoord commented Jan 29, 2022

Pierre-Sassoulas Jan 29, 2022

Choose a reason for hiding this comment

matusvalo commented Jan 29, 2022 • edited Loading

matusvalo commented Feb 3, 2022

DanielNoord left a comment

Choose a reason for hiding this comment

matusvalo commented Feb 4, 2022

jacobtylerwalls left a comment

Choose a reason for hiding this comment

matusvalo commented Feb 5, 2022

Pierre-Sassoulas commented Feb 6, 2022

coveralls commented Jan 14, 2022 •

edited

Loading

matusvalo commented Jan 15, 2022 •

edited

Loading

matusvalo commented Jan 26, 2022 •

edited

Loading

matusvalo commented Jan 29, 2022 •

edited

Loading