[WIP] [Refactor] Defining clippy rules #91
Conversation
checker/src/lib.rs
Outdated
| // TODO: Remove when fixed | ||
| clippy::result_unit_err |
There was a problem hiding this comment.
Added this one to by pass the Result<_, ()> linting as it needs more than a refactor I think
kaleidawave
added
the
internal-improvements
|
Awesome! looking good. Great addition to wrap a bunch of arguments to function calling. I always got a bit mixed up with which one was which, so that's a great fix. With the Also If you want to do the change to the checkers The tighter requirements seem great, going through the clippy page you linked and I think blocking them all by default will improve the codebase now it is getting more mature. You can update the line here to reflect that Line 59 in 5021262 |
|
Hey! About Also about: |
|
Btw, so you're aware of what |
|
Nice! Cool that it uncovered a bug in the parser as well. It is great that you have solved a lot of the current lints. For the 'pedantic' level lints it probably best to prioritise those changes until after 0.1 https://github.com/kaleidawave/ezno/milestone/1 Will check out the changes and merge on Friday when I have the time to go through it 👍 |
| @@ -63,13 +63,14 @@ fn markdown_lines_append_test_to_rust( | |||
| let blocks = { | |||
| let mut blocks = Vec::new(); | |||
| let mut current_filename = None; | |||
| while let Some((_, line)) = lines.next() { | |||
| for (_, line) in lines.by_ref() { | |||
There was a problem hiding this comment.
Nice! I have used .by_ref() on iterator methods that consume brefore. Didn't know it could be used in the case where you don't want for to consume 👍
| } | ||
|
|
||
| #[must_use] |
There was a problem hiding this comment.
Interesting. I haven't had much experience with #[must_use]. Is there a heuristic for when to it is needed? Was there any code that didn't use this result?
There was a problem hiding this comment.
According to this (old) reddit post by one of clippy maintainer here is how they decide which function should be marked with #[must_use]. Also, a lot of people were complaining about false positive, but it looks like it has been fixed and is working pretty well now.
After adding #[must_use], there was no additional error, so I guess everything was used
| environment, | ||
| arguments, |
There was a problem hiding this comment.
I wonder whether arguments could be added to the CallingInput? CallingInput already encompasses the sort of hidden arguments that a function call takes, I wonder whether actual values could also be passed here. Or is there a technical reason?
There was a problem hiding this comment.
There's a reason I didn't add it, it's because fn call in checker/types/callings takes arguments: &[SynthesisedArgument].
So if we want to add arguments in CallingInput, it'll mean that we need another struct with arguments only for this function. Both are fine I think, do you have a preference?
|
All changes look good. Just need to fix the something in the WASM section. Then will merge ! |
|
Fixed |
|
Awesome, just merged! |
Description
This PR aims to define standards for clippy, and fixes the
too_many_arugmenterror as well.About clippy
Imo the best strategy is to use
#![deny(clippy::all)]which is covering the most important linting rules, and to cherry pick some ofclippy::pedanticas using everything can be painful, and raise false positives.List of all the pedantic lintings
Example of some pendantic linting I think may be nice to have: