Refactor Fix and Edit API#4198
Conversation
|
The removal of |
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
Yeah, this is unfortunate but necessary because we'll need to explicitly pass the |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thank you for working on this refactor
Makes sense, just wanted to check before I really went down the rabbit hole on that one — I'll make those changes next and mark this as ready for review when it's done. |
|
@MichaReiser I've encountered a bit of a problem and since the work is rather repetitive I'd like to check in again :) In many cases, we can just replace However there are some cases where this is used in In this use, there's not a clear way to provide the specificity without requiring an Suggestions? Overall, this requires some careful editing as there are additional cases where a function is reused and probably should not decide the applicability level. My hopes of replacing these with regular expressions have gone down the drain 😁 Maybe it'd be easier to leave |
I would have been surprised if my proposal just worked 😆. Thanks for doing this work so carefully.
I agree, that this should be decided by the caller rather than by
In this case, I would create the diagnostic.try_set_fix(|| {
Ok(Fix::unspecified(
delete_stmt(
pass_stmt,
None,
&[],
checker.locator,
checker.indexer,
checker.stylist,
)?
))
});
I think it's fine if we get some of them wrong because we have to re-visit all of them anyway when deciding on the fixe's safety. We can use this opportunity to decide whether the function should defer the determination of the fixe's safety to the caller or even change the signature to return an The main motivation behind changing all of them to
That's' also be a valid approach that I haven't thought of. Determining the "fixes to migrate" workflow could then be:
I'll let you decide. You know the extent of the work best. |
|
Of course you can just use a closure :) my lack of experience with Rust is showing. I think that's a reasonable approach if you think it's maintainable. I'll forge onward but if the changes feel hard to review I'll consider the "separate changes" workflow some more. Thanks for the guidance again! |
|
@MichaReiser I am leaning towards separate pull requests for individual rules. Otherwise this is going to take a while longer. It'll be harder to review and I'll need to keep rebasing as old patterns are used on |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
@MichaReiser I am leaning towards separate pull requests for individual rules. Otherwise this is going to take a while longer. It'll be harder to review and I'll need to keep rebasing as old patterns are used on
main. I also wouldn't be upset if you wanted to take a look — familiarity with the codebase would probably make doing it all at once significantly easier.
Sounds good to me. We can merge the PR as is and do a couple of follow-up PRs where we migrate a bunch of .set_fix calls to use Fix::unspecified. That also gives us the option for multiple collaborators to contribute to the refactor, so that you don't have to do all by yourself.
Thank you
crates/ruff_cli/src/cache.rs
Outdated
| kind: header.kind, | ||
| range: header.range, | ||
| fix: header.fix, | ||
| fix: Some(header.fix), |
There was a problem hiding this comment.
MessageHeader is a helper struct to deserialize a Message. Its fields must match the fields on Message or the deserialization will fail if we have a Message without a fix.
That's why we should change the fix definition on MessageHeader to Option<Fix> to match Message
#[derive(Deserialize)]
struct MessageHeader {
kind: DiagnosticKind,
range: TextRange,
fix: Option<Fix>,
file_id: usize,
noqa_row: TextSize,
}There was a problem hiding this comment.
Done in 310fa94 — do you normally add tests for things like this to ensure it stays up to date?
There was a problem hiding this comment.
Thx. We probably should but don't have to as part of this PR.
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thank you, this is awesome.
2 participants
Part of #4181
Closes #4182
Thanks for the well specified issue! This implements many of the noted changes (see commit messages). There are a couple of deviations:
Fix::emptyMessage::fixto beOption<Fix>MessageHeader::fixto beOption<Fix>From<Edit> for Fix(see discussion)