The code is hard to read/follow about cargo-msrv HOT 1 CLOSED

Thanks for all your feedback! 😄

You comments gave me some food for thought today; I enjoy thinking about the structure and architecture on any level of software engineering, and I'm always open to improvements in the code or architecture.

I'll try to sincerely address each of your points below.

• the subcommands implement a SubCommand trait for no apparent reason? at least for verify it can just be removed without breaking anything (see 2053c7a) ... it was introduced in 94fedd1 "to enhance testing capability"

By using a trait, it can be replaced with a fake implementation, since mocks in Rust commonly add lots of #[cfg_attr(..., ...)] macro's all over the place, and sometimes they also require traits.
That's probably what my commit message "to enhance testing capability" refers to.

I also like that each subcommand has the same structure, which the trait enforces.

That being said, being at the top-level (at least for now), it is rarely necessary to replace the structs for which the SubCommand trait with fake implementations.

• the subcommands don't report their success value via the T in Result<T, E> but instead via a Reporter?
  

  cargo-msrv/src/sub_command/verify.rs

  Lines 83 to 86 in feef0f1

  	fn success(reporter: &impl Reporter, toolchain: ToolchainSpec) -> TResult<()> {
  	reporter.report_event(VerifyResult::compatible(toolchain))?;
  	Ok(())
  	}

The Result and Reporter have different use cases. The Reporter is used for reporting status updates, and sending all the data which is necessary to create and incrementally update a view for the user. It makes it easier to separate the program's logic from the user view.

In addition, the program doesn't just report its result at the end, but also on runtime. Since this program is written without async, it is not always possible, with the current architecture, to wait for a return. The user needs to be informed of the progression before a return happens.

Verify is not the best example, because it only reports one result at the end, although earlier steps like fetching the rust releases index are reported too (but this is not part of the SubCommand trait implementation). Find would be a better example, as it reports intermediate steps of a search process.

• errors are reported twice in the E in Result<T, E> as well as via the Reporter?
  

  cargo-msrv/src/sub_command/verify.rs

  Lines 95 to 99 in feef0f1

  	reporter.report_event(VerifyResult::incompatible(toolchain, error))?;
  	Err(CargoMSRVError::SubCommandVerify(Error::VerifyFailed(
  	VerifyFailed::from(rust_version),
  	)))

The returned errors from the Result values, are instead used for expected and unexpected internal errors, although errors may bubble up, and can be reported in the end.

I intended to separate expected and unexpected errors from each other by reporting expected errors (e.g. when a toolchain of a certain version fails to compile a project), from unexpected errors, but there is room for improvement here.

• the verify_msrv function takes &impl Check but is apparently only ever called with the RustupToolchainCheck implementation ... needless generics make the code harder to read

The idea here is that a different check command may be supplied. For example, some of the tests do this (it doesn't always make sense to have a complete toolchain ready to run a test case; instead a fake Check implementation can be used).

I've also been working on different Check implementations outside of tests. For example, I've been working on an implementation, where I do heuristic search on the source code itself, instead of downloading+running a complete toolchain using Rustup.

• There's a bunch of .try_into() going on which you cannot easily follow with rust-analyzer ... this could be rewritten as T::try_from(..).

Ah. I don't use Rust analyzer myself, so I can't really comment on the tool specifically.

I don't particularly dislike try_into myself; From my point of view, it is usually possible to figure out the resulting type in the near vicinity, for example from the return type. This usually this makes it easier to refactor, since you don't need to explicitly name concrete types at the call site.

That said, I'm all for readability. If the code is easier to understand, that's a big win in my book.

• The Context structs make it really difficult to understand where any of the configuration comes from.

In general, each subcommand has it's own context struct, built up from shared sub contexts. The subcommand context's contains configuration distilled from different types of configuration sources, and exists as the final configuration for just that subcommand.

For example, configuration comes from CLI arguments (via Clap), from Cargo manifest (Cargo.toml) or Cargo lock (Cargo.lock) files, from environment variables, from config files etc.

In case of cargo msrv verify, this the the VerifyContext, and in case of cargo msrv (find) this is the FindContext.

This configuration is also potentially hierarchical, that is, when one multiple options are present only one should be used and based on an ordering defined in advance (e.g. env var > cli flag > config file).

I like to query all necessary and available configuration at the start, so you can exit early when some configuration is wrong or missing.

Previously, I used one global Config struct, taken directly from Clap's arguments, but I found that, that made it harder to figure out which context switches were used by which command.

Even though I lazily loaded some information, it still contained far too much unnecessary information for, say the cargo msrv list subcommand, which operates from a completely different set of inputs compared to cargo msrv (find).

I will also note that, "The Context structs make it really difficult to understand where any of the configuration comes from" is, at least in part, intentional. It shouldn't matter for the program whether the configuration came from an environment variable or a cli flag.

That said, in the case of Verify, it comes from here, which is hooked up to the main pipeline from here.

I opened #752 to illustrate how I think the code could be made more readable.

I'll have a look at your suggestions, thanks!

from cargo-msrv.