Use Rule for FileNoqaDirectives#18966
Conversation
Summary -- See #18946 (comment). Test Plan -- Existing tests
ntBre
added
internal
CodSpeed Instrumentation Performance ReportMerging #18966 will not alter performanceComparing Summary
Benchmarks breakdown
|
|
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
I've a small perf recommendation to reduce the number of Rule::parse call
| }; | ||
|
|
||
| if *code == Rule::BlanketNOQA.noqa_code() { | ||
| let Ok(rule) = Rule::from_code(code) else { |
There was a problem hiding this comment.
Do we have a short-circuit path somewhere that skips iterating over all diagnostics if there are no noqa codes at all?
There was a problem hiding this comment.
I don't think so, but that's a good idea. I added
if file_noqa_directives.is_empty() && noqa_directives.is_empty() {
return Vec::new();
}to the top of this function, along with those two is_empty methods.
| } | ||
|
|
||
| if exemption.contains(code) { | ||
| if exemption.includes(rule) { |
There was a problem hiding this comment.
Should we add (or use) a method includes_code that accepts a secondary code instead of the rule. That would allow us to defer the rule parsing up to the point where we've found a suppression comment (which should be the exception).
To avoid calling Noqa::to_string, we can add a method Noqa::matches_code(code: &str) which can match the string without allocationg
There was a problem hiding this comment.
Ah yeah, I already added FileExemption::contains_secondary_code but I can call it before this to bail out earlier without parsing the rule code.
|
Hmm, I think I would have merged this into main or waited with merging to make it easier to review #18946 It's no big deal. It's just a bit confusing and distracting because I now need to decide for each change if I already reviewed it or not. The other advantage of merging to main is that it would give us better insight in how #18946 changes performance. Now it's all muddled up with this change. |
|
Oh, sorry about that! I wasn't thinking about the review or checking the benchmarks again, but it sounds obvious in hindsight. |
2 participants
Summary
See #18946 (comment).
Test Plan
Existing tests