Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per directory configs - preliminary changes #9550
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
base: main
Are you sure you want to change the base?
Per directory configs - preliminary changes #9550
Changes from all commits
e1a88bf
04d878c
9b3576f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I'd prefer to revert the changes in these two lines.
They are really rightly coupled to the final implementation of the per directory configs and the performance impact is hard to judge on its own. As far as I can see, all other changes in this PR are somewhat sensible on their own. This one isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should not be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why we need the merging of stats for the multi-dir config option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current state is that some stat counters are reset during
linter.open()
, some are reset inlinter.set_current_module
->init_single_module()
, and some are not reset at all (error, warning etc in 1st group, all counters in stats.by_module in 2nd group, statement in 3rd). It leads to incorrect score calculation when linter (i.e. main checker) is opened per file, or when it is opened after getting asts.So I decided to reset all possible counters for each new module by creating new LinterStats object in
set_current_module
.If stats reset is omitted entirely, then another problem arises:
When jobs>1, the same linter object can be used for checking several modules, stats after each module are copied and then merged.
It leads to a situation when some stats are accounted several times in final result (it's checked in
test_subconfigs_score
in my 1st PR).Explicit stats reset and merge in single process can be avoided, but it will require additional changes in code for parallel checks. I'd suggest to leave it as a possible optimization in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, you probably already explain it but I don't fully understand. This explanation seems to point to a general issue with stats merging, not something that has to do with multi-directory configs. Or am I misunderstanding you? If it is just a general issue we should tackle it in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we have unintended effects from not getting these within the context manager..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be down below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is really touching some of the core behaviour of this behemoth of a class so it is a bit hard to review. Sorry in advance.
Why is this now optional? I don't really like that design as it further complicates this function body. Could you explain why this is needed? And could that perhaps be a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linked report traces the regression to the call to
resolve()
.I wonder if we can guard it under
not path.is_absolute()
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: Seems like is_absolute() will always be false as things stand, so we probably need to look higher up the stack for a place to do some sort of conversion.