Explain version check results #178
Conversation
src/check.rs
Outdated
| CheckResult::WrongVersion => { | ||
| println!("🛑 {} (version requirements are not satisfied)", name) | ||
| } | ||
| CheckResult::NotFound => println!("❌ {} (not found)", name), |
There was a problem hiding this comment.
At least let's print the minimum required version, also how to install/update. Also, a missing probe-rs may or may not be a hard error depending on the project requirements, and the message printed should reflect that.
There was a problem hiding this comment.
Also somewhat relates to #171
but yeah - a hint if probe-rs is not really needed might help
| requirements_unsatisfied |= print_result( | ||
| "espflash", | ||
| check_version(espflash_version, 3, 3, 0), | ||
| "minimum required version is 3.3.0 - see https://crates.io/crates/espflash", |
There was a problem hiding this comment.
| "minimum required version is 3.3.0 - see https://crates.io/crates/espflash", | |
| "minimum required version is 3.3.0 - see https://crates.io/crates/espflash", |
Not sure on the wording here, older versions of espflash might also work but may have some bugs or missing features.
There was a problem hiding this comment.
True - but it's probably hard to explain these details here - we might (or might not) want to explain those things in the book
src/check.rs
Outdated
| if probe_rs_required { | ||
| "minimum version required is 0.25.0 - see https://probe.rs/docs/getting-started/installation/ for how to upgrade" | ||
| } else { | ||
| "minimum suggested version is 0.25.0 - see https://probe.rs/docs/getting-started/installation/ for how to upgrade" | ||
| }, | ||
| if probe_rs_required { | ||
| "not found - see https://probe.rs/docs/getting-started/installation/ for how to install" | ||
| } else { | ||
| "not found - while not required see https://probe.rs/docs/getting-started/installation/ for how to install" | ||
| }, |
There was a problem hiding this comment.
We can do a lot better than this mess of duplicated strings. Make print_result take impl AsRef<str> and the you can use format! to collapse these.
The "while not required" wording is a bit unfortunate, as a user I would question whether this is an ad or something, and why would I install something that I don't need?
There was a problem hiding this comment.
We can do a lot better than this mess of duplicated strings. Make print_result take impl AsRef and the you can use format! to collapse these.
mhhh - not too sure format! will make it much nicer and not just harder to read 🤔
Yeah - I somewhat struggled with the "not really required but maybe you want it" wording - not happy about it but ... Any suggestion? - we could certainly explain a lot of things here but probably we should avoid a wall-of-text - ideally, we will explain why, when and how to use it in the book (but we are not there yet)
Thinking about it I'm not sure we should have the links etc. at all here - just link to the book if any of the required / recommended checks fail
There was a problem hiding this comment.
I personally find this preferable to two separate strings, especially that we have at least 3 such decision in here, and you also use this pattern to print which toolchain is in question ("Rust ({rust_toolchain})").
let suggestion_kind = if probe_rs_required { "required" } else { "suggested" };
format!("minimum {suggestion_kind} version is 0.25.0 - see https://probe.rs/docs/getting-started/installation/ for how to upgrade");
3 participants
Closes #175
I was thinking about giving more details (the required version and/or the actual command executed) but maybe that is too much noise and not too helpful
Anyways just relying on emojis was a bit too brief