Refactor: migrate diagnostics to use trait rather than macro #256
Conversation
This commit makes it so that diagnostic checkers no longer have to define magic public variables which will be called by a macro. Instead, they all implement a common trait which will easily enforce that all the checking data is defined. This also means compile errors will be quicker and more understandable.
|
When I first designed the checker, I essentially structured it in this way. The current question is whether I should redesign the checker in an oo style, and if there are other linter examples that make this change more compelling. |
|
In my opinion it makes more sense this way, but it is your decision of course. The previous macro way was essentially just manually specifying what the compiler would do anyway (with a trait) |
| let file_id = context.get_file_id(); | ||
| let diagnostic_index = db.get_diagnostic_index(); | ||
| let Some(diagnostics) = diagnostic_index.get_diagnostics(&file_id) else { | ||
| return; |
There was a problem hiding this comment.
Why remove Option<()>? That way I can’t '?'
There was a problem hiding this comment.
True, but generally returning nothing is better because option introduces added complexity (must use unwrap, is_some, etc.). It doesn't make sense just to add a pointless option return value for this syntax, especially since we can just return early like in the above. Option should be used if it makes semantic sense
|
I referred to Clippy, which has a very similar structure, and perhaps this makes sense. |
2 participants
This commit makes it so that diagnostic checkers no longer have to define magic public variables which will be called by a macro. Instead, they all implement a common trait which will easily enforce that all the checking data is defined. This also means compile errors will be quicker and more understandable.