wert007 / commit-analyzer

Gets the time somebody worked on something from `git log`
MIT License
1 stars 1 forks source link

[Enhancement] Set Clippy to Maximum #26

Open kevinmatthes opened 2 years ago

kevinmatthes commented 2 years ago

Since we are linting the code, we should rather set Clippy to its most restrictive mode:

#![deny(clippy::all)]
#![deny(clippy::cargo)]
#![deny(clippy::complexity)]
#![deny(clippy::correctness)]
#![deny(clippy::nursery)]
#![deny(clippy::pedantic)]
#![deny(clippy::perf)]
#![deny(clippy::style)]
#![deny(clippy::suspicious)]

What do you think about that, @wert007?

kevinmatthes commented 2 years ago

This would be where my cargo-optimise would come in handy: run it twice about the crate root and the most things are done.

wert007 commented 2 years ago

Hmm, I just checked what problems clippy had with these options enabled and most where missing documentation (which is fair), or possible #[must_use] annotations, which are very nice for a library, but not necessary for such a small code base I think.

Since I also adviced you to enable them, mostly, so that you would write better rust code from the beginning, since you are relatively new to rust, I do not think this is really necessary.

kevinmatthes commented 2 years ago

But at least #![deny(clippy::style)] is a meaningful change, in my opinion.

wert007 commented 2 years ago

Hmm, it did not trigger on my end on the current code base.. So please feel free to call cargo clippy -- -D clippy::style 😄

kevinmatthes commented 2 years ago

By the way: making the functions return a compound type such as a 2-tuple of the intended value as well as a sysexits::ExitCode fixes the documentation-related complaints of Clippy. When the method succeeds, its sysexits::ExitCode would be sysexits::ExitCode::Ok and the value can be processed; else, it will be the corresponding exit code for the main function and the given value is a default return value to be discarded.

In Haskell, this is a pretty common procedure to indicate the return state of a purely functional function, i.e. without any monad magic.

How about this?

wert007 commented 2 years ago

Hmm, no, I do not think, that it is a good idea to replace Result<T, E> with (T, E). For one, you completely ignore the typical rust error handling, all warnings, lints, syntactic sugar of the language, and general design choices will not work anymore. Also the lint is right, it would be good to document under what circumstances a function returns the Err version of the result.