-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Make future-incompat-report output more user-friendly #9953
Conversation
r? @Eh2406 (rust-highfive has picked a reviewer for you, use r? to override) |
Seems pretty reasonable to me, thanks for this! One question I would have, although this is probably unrelated to this PR specifically, is what's going on here:
In your last code-block the |
@alexcrichton: I'm using the special |
Ah I see, makes sense! In that case I'd be happy to r+ this, but I'd like to confirm with @ehuss first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This seems to remove the suggestions on how to update from cargo report future-incompat
. Can those be retained? I suspect the normal behavior is that someone runs cargo build
, and gets the message to run with the flag or cargo report future-incompat
, and if they run the second command, they will never see the suggestions.
.collect::<Vec<_>>() | ||
.join("\n") | ||
}; | ||
let to_display = if config.shell().err_supports_color() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a small bug while reading this: #9960 (no need to fix it now, just making a note)
0424195
to
6818336
Compare
@ehuss: I've implemented your suggestions
I think that would be unnecessarily verbose - the full report is already pretty long. |
I think it is good to be concerned about the size and amount of detail in the report. However, I think the size increase is quite modest, and I think the information about newer versions is perhaps one of the most important parts of the message. Looking at a sample report of a typical hit on
It seems like providing guidance on how the user can potentially fix the issue is a very important part of the message. |
When the user enables `--future-incompat-report`, we now display a high-level summary of the problem, as well as several suggestions for fixing the affected crates. The command `cargo report future-incompatibilities` now takes a `--crate` option, which can be used to display a report (including the actual lint messages) for a single crate. When this option is not used, we display the report for all crates.
ef59695
to
6f18507
Compare
@ehuss - I've adjusted the code to same the computed update message along with the report, and to display it when the report is shown. |
@ehuss I've applied your feedback. |
@ehuss - I've applied your suggestions. The on-disk report now contains the same suggestions shown in the build output. |
Thanks! I appreciate you sticking with my finicky reviews. @bors r+ |
📌 Commit 7ffba67 has been approved by |
☀️ Test successful - checks-actions |
Update cargo 6 commits in c7957a74bdcf3b11e7154c1a9401735f23ebd484..7fbbf4e8f23e3c24b8afff541dcb17e53eb5ff88 2021-10-11 20:17:07 +0000 to 2021-10-19 02:16:48 +0000 - Make future-incompat-report output more user-friendly (rust-lang/cargo#9953) - Fix fetching git repos after a force push. (rust-lang/cargo#9979) - Add rustc-link-args to doctest build (rust-lang/cargo#9916) - Add the start of a basic benchmarking suite. (rust-lang/cargo#9955) - Use forms for issue templates. (rust-lang/cargo#9970) - Add rust_metadata to SerializedPackage (rust-lang/cargo#9967)
When the user enables
--future-incompat-report
, we now displaya high-level summary of the problem, as well as several suggestions
for fixing the affected crates.
The command
cargo report future-incompatibilities
now takesa
--crate
option, which can be used to display a report(including the actual lint messages) for a single crate.
When this option is not used, we display the report for all
crates.
Sample output from the
actix
crate:> RUSTFLAGS="-Z future-incompat-test" ~/repos/cargo/target/debug/cargo build -Z future-incompat-report
> RUSTFLAGS="-Z future-incompat-test" ~/repos/cargo/target/debug/cargo build -Z future-incompat-report --future-incompat-report -Z unstable-options
> RUSTFLAGS="-Z future-incompat-test" ~/repos/cargo/target/debug/cargo report future-incompatibilities -Z future-incompat-report --color never | head -n 100