Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't pass OutputFormat to Context anymore #971

Merged
merged 1 commit into from
Aug 9, 2024
Merged

Don't pass OutputFormat to Context anymore #971

merged 1 commit into from
Aug 9, 2024

Conversation

Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Aug 8, 2024

It seems the only place where one of the commands actually uses the OutputFormat is currently here:

Outcome::Failure(_) if ctx.user_output.output_format == OutputFormat::None => {

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 foresterre marked this pull request as ready for review August 8, 2024 12:07
@foresterre
Copy link
Owner

foresterre commented Aug 8, 2024

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.

Copy link

codecov bot commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@foresterre
Copy link
Owner

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

It seems the only place where it was actually used was in `src/sub_command/verify.rs`.
And even there it was probably not needed since the reporter for 'no output'
would most likely discard the message anyway.

Not passing the OutputFormat to Context simplifies the code, and is probably
also cleaner, since the Context and the commands likely should not care about
the output format. The main code will set up reporting according to the output
format.
@Marcono1234
Copy link
Contributor Author

Have rebased the changes now.

@foresterre foresterre merged commit 8661116 into foresterre:main Aug 9, 2024
15 checks passed
@Marcono1234 Marcono1234 deleted the context-no-output-format branch August 9, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants