Issue 1554: add progress reporting

[jzohrab]

Add simple progress reporting to single-core pylint runs, per #1554

Simplified code changes after the discussion from draft PR #10134 (thanks)

As requested, the PR adds simple progress reporting to the --verbose flag. Sample output, as given in the test case:

Using config file pylint/testutils/testing_pylintrc
Get ASTs.
tests/regrtest_data/wrong_import_position.py
tests/regrtest_data/import_something.py
Linting 2 modules.
tests/regrtest_data/wrong_import_position.py (1 of 2)
************* Module wrong_import_position
{module2}:11:0: C0413: Import "import os" should be placed at the top of the module (wrong-import-position)
tests/regrtest_data/import_something.py (2 of 2)

Type of Changes

	Type
✓	✨ New feature

Notes

1. Partially closes Add an option to generate progress output #1554. This PR doesn't address progress reporting for parallel processing: a) the current code has some notes implying that the parallel processing will be merged in with the other "step 3" code; b) adding single-core reporting is a step forward that might be useful for many users.

2. The single test added in this branch passes; however, CI for this branch will fail, because the branch is based off of main which currently has several failing tests. On my local machine:

branch	pytest results
upstream/main as at d94194b	30 failed, 1885 passed, 264 skipped, 5 xfailed in 122.39s (0:02:02)
this branch	30 failed, 1886 passed, 264 skipped, 5 xfailed in 124.96s (0:02:04)

3. Adding this feature may alter some of the existing 30 failures, due to the changed --verbose output. I had suggested that progress reporting be separated out from --verbose for this reason, but have left it as part of the --verbose because I can understand the desire for fewer flags. I haven't looked into any new failures much as they're buried in the existing test failures.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.84%. Comparing base (d94194b) to head (a4489ba).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10178   +/-   ##
=======================================
  Coverage   95.83%   95.84%           
=======================================
  Files         174      175    +1     
  Lines       19002    19028   +26     
=======================================
+ Hits        18211    18237   +26     
  Misses        791      791           

Files with missing lines	Coverage Δ
pylint/config/config_initialization.py	98.90% <100.00%> (+0.01%)	⬆️
pylint/lint/pylinter.py	96.71% <100.00%> (+0.04%)	⬆️
pylint/reporters/progress_reporters.py	100.00% <100.00%> (ø)

[Pierre-Sassoulas]

Thank you ! The reporter classes might be better suited for the pylint.reporter package ? Or would we be mixing unrelated reporters ?

[Pierre-Sassoulas]

If it's not in pylint.reporter I think it's properly located right now in pylint.lint.progress_reporter

[DanielNoord]

I think having it in the reporters directory makes sense. That would be the place I would go looking.

[Pierre-Sassoulas]

My first instinct was pylint.reporter.progress_reporter.py

[Pierre-Sassoulas]

Added skip news because this is the first step for progress report, but we can add this to the changelog if you want @jzohrab. Great first contribution and you've been very mindful of getting the design right.

[DanielNoord]

@Pierre-Sassoulas Do you agree with setting the progress reporter as an instance attribute of PyLinter so we don't need to pass len(ast_mapping) and can instantiate the thing once?

[jzohrab]

My design notes only: Instantiating costs nothing and adding it as a class member means that progress reporter code is spread more around the class, instead of being created and used within a small and easily understood context. With how it is now, there is less state overall, and so is likely easier to deal with later. The only thing I can think of that would make it better would be a _progress_reporter factory method in the pylinter, to centralize the instantiation, but that feels like too much for such a small thing :-)

[jzohrab]

(Hit Send too fast)

That said, happy to change as needed to make proj owners happy, and maybe having it as a member allows for other things later on.

[Pierre-Sassoulas]

I don't have a strong opinion either way. As I looked for a long time at this PR to come to this conclusion, a nitpick came to me : maybe the reporter class could use the same naming and behavior than the existing BaseReporter with an injectable output buffer:

pylint/pylint/reporters/base_reporter.py

Line 31 in 80f2946

def __init__(self, output: TextIO | None = None) -> None:

pylint/pylint/reporters/base_reporter.py

Line 34 in 80f2946

self.out: TextIO = output or sys.stdout

pylint/pylint/reporters/base_reporter.py

Line 43 in 80f2946

def writeln(self, string: str = "") -> None:

But really, I think we don't have to polish this too much. There's a ton of issues that would be more beneficial to work on than thinking really hard about a progress reporter design. As it is it's simple, it's required to display progress, and it's going to work.

[DanielNoord]

Awesome, thanks so much @jzohrab!

[jzohrab]

Awesome, thanks so much @jzohrab!

🎉🎉🎉 super and I hope all the users like it. 👋

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit a4489ba