fix "skipped files" count calculation #10141
Conversation
purajit
changed the title
purajit
force-pushed
the
20241214-skipped-files-count
branch
4 times, most recently
from
feeb8f5 to
b29b3b6
Compare
purajit
changed the title
purajit
changed the title
| @@ -51,6 +51,7 @@ class ModuleDescriptionDict(TypedDict): | |||
| isarg: bool | |||
| basepath: str | |||
| basename: str | |||
| isignored: bool | |||
There was a problem hiding this comment.
Was initially hoping to have all ignored files reporting go through this, but that
seems like it would be a larger refactor and out of scope.
There was a problem hiding this comment.
Thank you this look great. Agree that there is a large refactor to do it we want to have a verbose mode that make sense (display what will be skipped or not before the analysis and not after). I don't want to block bug fixes waiting for a refactor I don't have any bandwith for however.
This comment has been minimized.
This comment has been minimized.
|
Resolved the tests. I've decided not to walk the tree to find the precise number of skipped files since it can get Some of the failed tests as well as the ^ message above are both seemingly related to the os.walk |
|
Ah, I think astroid's |
pylint/lint/pylinter.py
Outdated
| @@ -851,6 +852,7 @@ def _get_file_descr_from_stdin(self, filepath: str) -> Iterator[FileItem]: | |||
| self.config.ignore_patterns, | |||
| self.config.ignore_paths, | |||
| ): | |||
| self.skipped_paths.add(filepath) | |||
There was a problem hiding this comment.
Can we instead store this as a an attribute on LinterStats? Preferably just as an int as we never actually need the file name. That would centralise all statistics counting.
There was a problem hiding this comment.
I had that originally! I had two concerns that made me switch
- the ignore logic happens at a few different layers. From the way it's structured, there should be no overlap, but I was weary of double-counting
- in the next step (or potentially in this PR itself, depending on how you feel) I'd also like to use
get_module_filesto get an accurate file count rather than a βfile or module" count; it would also enable pylint to report precisely what got skipped, though I guess that could be reported as we iterate and ignore them
If you're cool with the first, I can switch it back and leave the second point to a future discussion.
There was a problem hiding this comment.
Yes, I think the first is fine. If it turns out we are double counting we would have an actual reproducer and know what the bug is. Let's not optimize for something that might not happen.
Just doing a quick confirmation with @Pierre-Sassoulas that we are fine with doing an initial PR where we just store this on LinterStats. I don't want to you to change the code and then have another maintainer request the exact opposite.
As for the second change, I agree: let's do that in a follow up.
There was a problem hiding this comment.
Sorry, was waiting on Pierre's response, then got busy - pushed an update; ran all tests locally and reconfirmed that everything worked.
There was a problem hiding this comment.
Sorry for missing that. We can do it in linterstat I don't want to block bug fixes for a refactor I don't have bandwith for. (But this is going to be nuked if/when we refactor).
There was a problem hiding this comment.
Yep, I've pushed the linterstat version!
This comment has been minimized.
This comment has been minimized.
purajit
force-pushed
the
20241214-skipped-files-count
branch
from
54aeecd to
f08024b
Compare
There was a problem hiding this comment.
Thank you for being persistent :) It'll need a changelog there: https://github.com/pylint-dev/pylint/tree/main/doc/whatsnew/fragments
create a news fragment with towncrier create . which will be included in the changelog. can be one of the types defined in ./towncrier.toml.
Codecov ReportAll modified and coverable lines are covered by tests β
Additional details and impacted files@@ Coverage Diff @@
## main #10141 +/- ##
=======================================
Coverage 95.83% 95.83%
=======================================
Files 174 174
Lines 18995 19002 +7
=======================================
+ Hits 18204 18211 +7
Misses 791 791
|
This comment has been minimized.
This comment has been minimized.
(cherry picked from commit d94194b)
|
π€ According to the primer, this change has no effect on the checked open source code. π€π This comment was generated for commit d3cb8de |
(cherry picked from commit d94194b)
|
Congrat on becoming a pylint contributor ! We're going to release this in 3.3.4 |
Type of Changes
Description
Closes #10073. This bug has existed ever since the logic was introduced in #9122. This was
the initial issue that inspired that PR, so we can see that the "skipped" count should obviously
have nothing to do with modules without docstrings.
Also enables more reports/output about skipped files in the future, which has been asked for.
I haven't implemented that since it would probably be a larger discussion as to how verbose
--verboseshould be.Testing
Fleshed out the test that was written when this was introduced that didn't actually test anything.
Ran this with several configs, including overlapping paths and modules. Additionally ran it on
our biggest repo with multiple packages, vendored code, etc, and got the expected results:
Before:
Checked 10xxx files, skipped 9391 files(alarming!)After:
Checked 10xxx files, skipped 75 files/modules(whew, ok)