feat(update): Tell users when they are still behind #13372
Conversation
There might be casses where we want to show pre-release as this grows but, for now, there isn't too often a case where an update is held back, you are on a pre-release, your pre-release is updating, and you want a pre-release.
`--verbose` will show them. This is prep for telling the user about `--breaking` and other flags.
|
r? @weihanglo (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
A-testing-cargo-itself
|
CC @djc since we had conversations about related to this for |
For myself, I feel like the trailing `--invert` reads a little awkwardly. Noticed when copying the message for rust-lang/cargo#13372
I put this behind `--verbose` - To keep the output down in the standard case - Because its assuming most people's "behind" dependencies will be "Unchanged" and so that is when knowing how to look up how its pulled in is useful
epage
force-pushed
the
update-report
branch
2 times, most recently
from
48b55a3
to
dbca11f
Compare
There was a problem hiding this comment.
I really like this approach! I think showing this data in the update output relieves the pressure on having upgrade (or --breaking or whatever we spell it as).
Suggested some stylistic improvements, feel free to ignore of course.
I would personally appreciate showing unchanged behind dependencies even without -v but that might be too big a step?
| .map(|s| s.as_summary()) | ||
| .filter(|s| { | ||
| let s_version = s.version(); | ||
| s_version.pre.is_empty() | ||
| // Only match pre-release if major.minor.patch are the same | ||
| || (s_version.major == present_version.major | ||
| && s_version.minor == present_version.minor | ||
| && s_version.patch == present_version.patch) | ||
| }) | ||
| .map(|s| s.version().clone()) | ||
| .max() | ||
| .filter(|v| present.version() < v) | ||
| .map(|v| format!(" {warn}(latest: v{v}){warn:#}")) |
There was a problem hiding this comment.
Style suggestion: personally I find this kind of code easier to follow if you combine closures for map().filter().map() and filter().map() into one each.
There was a problem hiding this comment.
For me, its the opposite. Each one is serving a unique purpose and having them represented separately better shows that and is easier to refactor (e.g. I should turn the filter into a take_while). I also find filter for booleans easier to read than requiring converting bools to Option in a filter_map, whether through an explicit if or through .then.
I use filter_map more when I'm working with Options or the steps are more coupled.
| .into_iter() | ||
| .flatten() | ||
| .max_by_key(|s| s.version()); | ||
| let latest = if let Some(present) = highest_present.filter(|p| p.source_id().is_registry()) |
There was a problem hiding this comment.
Maybe use a filter_map() for this too, to avoid the dangling else { None } branch at the end? Could use early return.
Also given that latest is prefiltered to be distinct from the current actual, I find the name not that obvious. Maybe newer?
There was a problem hiding this comment.
In my experience, the right balance of combiantors vs procedural logic is a matter of whether its bookkeeping and size. In this case, this expression is too large in my opinion to shove in an iterator combinator.
| "To see how you depend on a package, run `cargo tree --invert --package <dep>@<ver>`", | ||
| )?; | ||
| } else { | ||
| if 0 < unchanged_behind { |
There was a problem hiding this comment.
Suggestion: I think writing unchanged_behind > 0 would be more idiomatic (and easier to parse for me, at least)?
And even then, it's a little unclear to me what this condition is about. Maybe add a comment?
There was a problem hiding this comment.
While there is yoda speak (constant == variable) and more general speak (variable == constant) for equality, I see comparison operators differently and prefer to write them in number line order as I find that easier to read.
There was a problem hiding this comment.
I am on the same side of djc regarding the order, but not really a blocker.
I think it is. People might not want to play dependency whack-a-mole with transitive dependencies and so overwhelming them with all of the unchanged-but-behind dependencies, especially in a large workspace, would be frustrating especially as it might hide the changes the user cares about. Or put another way, the primary purpose of the command is updating and the majority of the output should be devoted to that and not with "help". |
How about only showing them for direct dependencies, that is, dependency edges that originate in the current workspace? |
|
In a large workspace, there can still be a lot of direct dependencies. |
|
IMO when you are running |
|
Thanks! @bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 14 commits in cdf84b69d0416c57ac9dc3459af80dfb4883d27a..ccc84ccec4b7340eb916aefda1cb3e2fe17d8e7b 2024-02-02 19:39:16 +0000 to 2024-02-07 15:37:49 +0000 - Relax a test to permit warnings to be emitted, too. (rust-lang/cargo#13415) - test: disable lldb test as it requires privileges to run on macOS (rust-lang/cargo#13416) - Update git2 (rust-lang/cargo#13412) - fix: Switch more notes/warnings to lowercase (rust-lang/cargo#13410) - Don't add the new package to workspace.members if there is no existing workspace in Cargo.toml. (rust-lang/cargo#13391) - Remove build metadata from curl-sys version. (rust-lang/cargo#13401) - Fix markdown line break in cargo-add (rust-lang/cargo#13400) - Remove `package.documentation` from the “before publishing” list. (rust-lang/cargo#13398) - chore(deps): update gix (rust-lang/cargo#13380) - chore(deps): update compatible (rust-lang/cargo#13379) - feat(update): Tell users when they are still behind (rust-lang/cargo#13372) - docs(changelog): Slight cleanup (rust-lang/cargo#13396) - Bump to 0.79.0; update changelog (rust-lang/cargo#13392) - doc: `[package]` doesn't require `version` field (rust-lang/cargo#13390) r? ghost
What does this PR try to resolve?
Part of this is an offshoot of #12425 which is about pulling some of
cargo upgrades behavior intocargo update. One of the "'Potential relatedcargo updateimprovements" is informing the user when they are behind.Part of this is to help close the gap of users being behind on their dependencies unaware. This is commonly raised when discussing an MSRV-aware resolver (see rust-lang/rfcs#3537) but breaking changes are just as big of a deal so I'm starting this now.
See also #7167, #4309
Compared to
cargo upgrade/cargo outdated, I'm taking a fairly conservative approach and tweaking the existing output as a starting point / MVP. We can experiment with a richer or easier-to-consume way of expressing this over time.I view us telling people they aren't on the latest as a warning, so I made that text yellow.
clap $ cargo update --dry-runclap $ cargo update --dry-run --verboseHow should we test and review this PR?
This sets up the minimal implementation and slowly adds bits at a time, with a test first that demonstrates it.
Additional information
I'm expecting that the
cargo upgradeintegration will extend the notes to say something like "X dependencies may be updated with--breaking"