fix: correct error count for cargo check --message-format json
#14598
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
84a0257
to
d9f9619
Compare
Welcome! If you ever want some face-time with a Cargo team member, we hold office hours on jitsi, see https://github.com/rust-lang/cargo/wiki/Office-Hours It would be great if this change could have a test! Looking around, my guess is one of the following files will have relevant tests to put this next to: Something we've found very helpful when writing tests is to add them in a commit before a fix, with every commit passing the test. See https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request for more details.
We don't have an official style though some of us use Conventional Commits |
|
Thanks for providing the information! I would add the test for this modification. |
90f84e3
to
34b3326
Compare
|
I think the original test already cover this PR, instead of creating one, I modify the test to make it produce the correct result. |
| } | ||
| if let Ok(message) = serde_json::from_str::<CompilerMessage>(compiler_message.get()) { | ||
| count_diagnostic(&message.level, options); | ||
| if let Ok(msg) = serde_json::from_str::<CompilerMessage>(compiler_message.get()) { |
There was a problem hiding this comment.
Instead of reusing a larger struct definition, should we just add an extra field message, and change count_diagnostic to accept three args (level, message, options) ?
Although having no data, my intuition tells me that skipping unused fields is generally more performant.
{
"rendered": "error: expected one of `!` or `::`, found `<eof>`\n --> src/main.rs:3:1\n |\n3 | xxx\n | ^^^ expected one of `!` or `::`\n\n",
"children": [], // may contain nested diagnostics
"level": "error",
"message": "expected one of `!` or `::`, found `<eof>`",
}compares to
{
"level": "error",
"message": "expected one of `!` or `::`, found `<eof>`",
}There was a problem hiding this comment.
should we just add an extra field message, and change
count_diagnosticto accept three args(level, message, options)?
Do you mean we should check the message in count_diagnostic instead of checking before passing the (level, options) to count_diagnostic?
There was a problem hiding this comment.
Ignore the count_diagnostic part. I have no idea what I was talking about.
I meant that we may want to keep a separate CompilerMessage struct definition, since the current patch deserializes more fields than needed.
34b3326
to
71c830c
Compare
|
Awesome! Thanks for helping. @bors r+ |
|
☀️ Test successful - checks-actions |
Update cargo 19 commits in eaee77dc1584be45949b75e4c4c9a841605e3a4b..80d82ca22abbee5fb7b51fa1abeb1ae34e99e88a 2024-09-19 21:10:23 +0000 to 2024-09-27 17:56:01 +0000 - Update cc to 1.1.22 (rust-lang/cargo#14607) - feat: lockfile path implies --locked on cargo install (rust-lang/cargo#14556) - feat(toml): Add `autolib` (rust-lang/cargo#14591) - fix: correct error count for `cargo check --message-format json` (rust-lang/cargo#14598) - test: relax panic output assertion (rust-lang/cargo#14602) - feat(timings): support dark color scheme in HTML output (rust-lang/cargo#14588) - feat: add CARGO_MANIFEST_PATH env variable (rust-lang/cargo#14404) - fix(config): Don't double-warn about `$CARGO_HOME/config` (rust-lang/cargo#14579) - fix(cargo-rustc): give trailing flags higher precedence on nightly (rust-lang/cargo#14587) - feat: make lockfile v4 the default (rust-lang/cargo#14595) - perf: Improve quality of completion performance traces (rust-lang/cargo#14592) - test: Remove completion tests (rust-lang/cargo#14590) - feat: Add support for completing `cargo update <TAB>` (rust-lang/cargo#14552) - test: Migrate remaining with_stdout/with_stderr calls (rust-lang/cargo#14577) - fix(resolve): Improve multi-MSRV workspaces (rust-lang/cargo#14569) - chore: Bump MSRV to 1.81 (rust-lang/cargo#14585) - Add a `--dry-run` flag to the `install` command (rust-lang/cargo#14280) - fix(resolve): Don't list transitive, incompatible dependencies as available (rust-lang/cargo#14568) - feat(complete): Upgrade clap_complete (rust-lang/cargo#14573)
Hi! This is my first time contributing to Cargo. If there is anything that I need to do, please let me know!
(I'm not sure whether the commit message is aligned with the Cargo's convention. If it doesn't, I'm willing to modify it!)
This PR resolves the issue with incorrect error count and ensures warnings are correctly displayed when using
cargo check --message-format json.Fixes #14472