Skip to content

Conversation

@ntBre
Copy link
Contributor

@ntBre ntBre commented Jun 4, 2025

Summary

This is a spin-off from #18447 (comment) to avoid using Message::noqa_code to differentiate between lints and syntax errors. I went through all of the calls on main and on the branch from #18447, and the instance in ruff_server noted in the linked comment was actually the primary place where this was being done. Other calls to noqa_code are typically some variation of message.noqa_code().map_or(String::new, format!(...)), with the major exception of the gitlab output format:

let (description, check_name) = if let Some(code) = message.to_noqa_code() {
(message.body().to_string(), code.to_string())
} else {
let description = message.body();
let description_without_prefix = description
.strip_prefix("SyntaxError: ")
.unwrap_or(description);
(
description_without_prefix.to_string(),
"syntax-error".to_string(),
)
};

which obviously assumes that None means syntax error. A simple fix here would be to use message.name() for check_name instead of the noqa code, but I'm not sure how breaking that would be. This could just be:

 let description = message.body();
 let description = description.strip_prefix("SyntaxError: ").unwrap_or(description).to_string();
 let check_name = message.name();

In that case. This sounds reasonable based on the Code Quality report format docs:

Name Type Description
check_name String A unique name representing the check, or rule, associated with this violation.

Test Plan

Existing tests

@ntBre ntBre requested review from MichaReiser and dhruvmanila June 4, 2025 20:33
@ntBre ntBre added internal An internal refactor or improvement diagnostics Related to reporting of diagnostics. labels Jun 4, 2025
@ntBre ntBre marked this pull request as ready for review June 4, 2025 20:33
@github-actions
Copy link
Contributor

github-actions bot commented Jun 4, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

This looks pretty straightforward!

Comment on lines 277 to 267
code: rule.noqa_code().to_string(),
code: code?.to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

nit: we could short-circuit if code is None and avoid generating the edits although I think currently it's only the syntax errors that doesn't have code and those doesn't have fixes, so maybe this isn't that useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think the fix.is_some() || noqa_edit.is_some() condition should virtually guarantee this is never None, but I can move it to the top of the closure just in case. Thanks!

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.

Sweet

@ntBre ntBre enabled auto-merge (squash) June 5, 2025 12:48
@ntBre ntBre merged commit 74a4e9a into main Jun 5, 2025
33 checks passed
@ntBre ntBre deleted the brent/combine-lsp-diagnostic branch June 5, 2025 12:50
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.

4 participants