Convert OldDiagnostic::noqa_code to an Option<String>#18946
Conversation
ntBre
added
internal
|
|
The performance is a bit worse (-2% in the worst case) on the all-rules benchmarks, but it seems widely dispersed, so I think it may just be from the up-front |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
This mostly looks good. I think we want to keep using Rule in noqa.rs. But I'm not sure if that's better done in a separate PR
I also suggest introducing a SecondaryCode struct. It's hard to tell what all the &str and String usages are (especially because there are so few comments)
crates/ruff/src/printer.rs
Outdated
| .fold( | ||
| vec![], | ||
| |mut acc: Vec<((Option<NoqaCode>, &OldDiagnostic), usize)>, (code, message)| { | ||
| |mut acc: Vec<((Option<&str>, &OldDiagnostic), usize)>, (code, message)| { |
There was a problem hiding this comment.
I wonder if we should add a SecondaryCode struct which wraps a String I do find the &str makes the code harder to understand.
There was a problem hiding this comment.
Hmm, I think if it wrapped a String we'd have to clone in OldDiagnostic::secondary_code right? I'll try it with a &str and see if we can use that everywhere. I definitely agree that it would be more readable, though.
There was a problem hiding this comment.
I think I was responding to these too early in the morning 🤦 I can store the SecondaryCode(String) on the OldDiagnostic and then use &SecondaryCode where I'm currently using &str. I'm working on that, and then I'll try the Rule changes on a separate branch!
Co-authored-by: Micha Reiser <[email protected]>
Co-authored-by: Micha Reiser <[email protected]>
|
Summary -- See #18946 (comment). Test Plan -- Existing tests
Summary -- See #18946 (comment). Test Plan -- Existing tests
2 participants
Summary
I think this should be the last step before combining
OldDiagnosticandruff_db::Diagnostic. We can't store aNoqaCodeonruff_db::Diagnostic, so I converted thenoqa_codefield to anOption<String>and then propagated this change to all of the callers.I tried to use
&streverywhere it was possible, so I think the remainingto_stringcalls are necessary. I spent some time trying to convert everything to&strbut ran into lifetime issues, especially in theFixTable. Maybe we can take another look at that if it causes a performance regression, but hopefully these paths aren't too hot. We also avoid someto_stringcalls, so it might even out a bit too.Test Plan
Existing tests