Skip to content

Change NoqaCode to a single string with an offset #18736

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ntBre
Copy link
Contributor

@ntBre ntBre commented Jun 17, 2025

Summary

This PR explores two potential and closely related NoqaCode refactors. The first, in the first two commits, replaces the current NoqaCode(&'static str, &'static str) with NoqaCode(&'static str, usize), where the usize is the index where the prefix and suffix separate. As detailed in this comment, it's not really possible to get a nice, single &'static str, but we can hack around it with a LazyLock to call format! in a static:

static CODE: std::sync::LazyLock<String> = std::sync::LazyLock::new(
|| format!("{}{}", crate::registry::Linter::#linter.common_prefix(), #code));
NoqaCode(CODE.as_str(), crate::registry::Linter::#linter.common_prefix().len())

In the third commit, I just tried NoqaCode(String, usize), which means NoqaCode can no longer be Copy, but is otherwise a bit more natural.

Test Plan

Codspeed benchmarks on this PR. The &'static str version didn't show any difference, but the String version shows up to a 3% regression. Still, either of these representations should be a bit better if we end up needing to call Rule::from_code since they will avoid having to allocate a new string just to pass it there. A single string should be easier to store on ruff_db::Diagnostic too.

impl Rule {
pub fn from_code(code: &str) -> Result<Self, FromCodeError> {
let (linter, code) = Linter::parse_code(code).ok_or(FromCodeError::Unknown)?;

@ntBre ntBre changed the title Change NoqaCode to a single string with an offset Change NoqaCode to a single string with an offset Jun 17, 2025
Copy link
Contributor

github-actions bot commented Jun 17, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@ntBre ntBre added the internal An internal refactor or improvement label Jun 17, 2025
@MichaReiser
Copy link
Member

I wonder if this refactor is worth it, considering that it's a slow down. Should we wait until we add secondary_code: String to ruff_db::Diagnostic and then take this up again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants