Skip to content

Conversation

@ntBre
Copy link
Contributor

@ntBre ntBre commented Jun 26, 2025

Summary

See #18946 (comment).

Test Plan

Existing tests

Summary
--

See #18946 (comment).

Test Plan
--

Existing tests
@ntBre ntBre added internal An internal refactor or improvement diagnostics Related to reporting of diagnostics. labels Jun 26, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Jun 26, 2025

CodSpeed Instrumentation Performance Report

Merging #18966 will not alter performance

Comparing brent/directive-rule (2bbc37e) with brent/string-noqa-code (4eab76b)

Summary

✅ 37 untouched benchmarks
⁉️ 1 dropped benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
⁉️ ty_micro[many_attribute_assignments] 56.6 ms N/A N/A

@github-actions
Copy link
Contributor

github-actions bot commented Jun 26, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@ntBre ntBre marked this pull request as ready for review June 26, 2025 17:17
@ntBre ntBre requested a review from MichaReiser June 26, 2025 17:18
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a short-circuit path somewhere that skips iterating over all diagnostics if there are no noqa codes at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ntBre ntBre merged commit 12e971f into brent/string-noqa-code Jun 26, 2025
34 of 35 checks passed
@ntBre ntBre deleted the brent/directive-rule branch June 26, 2025 21:01
@MichaReiser
Copy link
Member

MichaReiser commented Jun 27, 2025

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.

@ntBre
Copy link
Contributor Author

ntBre commented Jun 27, 2025

Oh, sorry about that! I wasn't thinking about the review or checking the benchmarks again, but it sounds obvious in hindsight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

diagnostics Related to reporting of diagnostics. internal An internal refactor or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants