search

foresterre / cargo-msrv

🦀 Find the minimum supported Rust version (MSRV) for your project
https://foresterre.github.io/cargo-msrv
Apache License 2.0
895 stars 29 forks source link

Don't pass `OutputFormat` to `Context` anymore #971

Closed Marcono1234 closed 3 months ago

Marcono1234 commented 3 months ago

It seems the only place where one of the commands actually uses the OutputFormat is currently here: https://github.com/foresterre/cargo-msrv/blob/ab2870f41a8bace2cba5c63a3dfa17e534e4a903/src/sub_command/verify.rs#L76

And from my testing it looks like this is redundant because if the output format is NONE, then the output handler will ignore all output anyway, so it does not matter whether error_message is passed along or not (but please verify yourself, if you want).

This pull request is a proof of concept, showing that all the OutputFormat handling code can therefore be removed from the Context code. This simplifies the code a bit, and also separates it more, because the context and command code probably does not care about the OutputFormat; cargo-msrv.rs handles all of that. What do you think?

The disadvantage is that if in the future for whatever reason the context or command code needs the output format again, it would have to be passed along again.

No problem if you don't think this change is useful. During my changes for #963 I was just curious whether it was possible to refactor the code in this way.

foresterre commented 3 months ago

I think if it simplifies the code, we should do it. If this changes in the future, we will add it back at such time.

I can see why it would be useful in a future where there's not just clap's argument parser to set these values but also environment variables and a config file. In this future, the Context acts as the merged immutable state, and is part of the public (Rust) API surface to the outside world (and can also be called from Rust code like this).

For that future to become true however, I will probably also need to setup the reporter after the Context, because it is the reporter which uses this value (i.e. to determine which reporter is to be instantiated).

Alternatively, the reporter can still be created outside of the lib.rs, but then there will be some output-format specific boilerplate to get the format from each of the different inputs inside the bin/cargo-msrv.rs.

Writing this down now, I think this may actually be preferred as it gives the caller of maximum power to configure a (custom) reporter themselves.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 75.09%. Comparing base (cf8347b) to head (8c01c59). Report is 7 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #971 +/- ## ========================================== - Coverage 75.17% 75.09% -0.09% ========================================== Files 80 80 Lines 5564 5530 -34 ========================================== - Hits 4183 4153 -30 + Misses 1381 1377 -4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

foresterre commented 3 months ago

I'll merge this after you rebased it; thanks for the PR!

Marcono1234 commented 3 months ago

Have rebased the changes now.