-
Notifications
You must be signed in to change notification settings - Fork 978
Remove more Notification variants #4501
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
base: master
Are you sure you want to change the base?
Conversation
1dcd5dc
to
15a9192
Compare
15a9192
to
a4dd0a3
Compare
a4dd0a3
to
ed4ff46
Compare
.await | ||
.with_stderr(snapbox::str![[r#" | ||
info: component 'rust-std' for target '[CROSS_ARCH_I]' is up to date | ||
info: component is up to date component=rust-std target=[CROSS_ARCH_I] |
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.
Are you sure about this? Message changes like this are making the readability worse, thus this might be a UX regression when it comes to info!()
, warn!()
and error!()
...
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.
Or, are you moving towards the machine-readable output this way? If that's the case I'm afraid we need to update our user-facing message format:
Line 112 in 7365ab7
ctx.field_format().format_fields(writer.by_ref(), event)?; |
... to make it clearer, say "info: component is up to date for component=rust-std target=[CROSS_ARCH_I]" to stay closer to the original vibe.
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.
Yeah, I was hoping to make this more like structured logging. However, I agree that this is a bit of a UX regression, so I'll revert some of the message changes to be more human-friendly.
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.
@djc Now that you've brought this up, I think there are actually two ways of doing #450. The first is giving the notification system an alternative output format (corrosion maintainer specifically asked for JSON for better CMake compatibility, so the resulting output would be JSONL), the second would be returning a specifically-designed single JSON value.
If we are moving on with the first option then your work of extracting field names is actually valuable (as I can then further abuse tracing_subscriber
to output JSON directly), so that is why I proposed the compromise above :)
What do you think about this?
cc @ChrisDenton
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 find the first option appealing as it seems much more generally useful and I think it's more in line with what tools such as cargo provide. I'm less sure about hacking around tracing_subscriber
to provide it but I'd want a better idea of what that would look like. Maybe it's ok.
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.
@ChrisDenton Back in the v1.28 release cycle I've reproduced the regular rustup log messages with tracing_subscriber
while retaining the previous format, and almost everyone should be using it now without major issues (some aren't even aware of the fact?).
My option 1 is just a natural extension of that idea.
Of course, special care must be taken to handle the direct prints in say rustup show
, but I consider that to be a minority.
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 don't really want to increase the scope of this PR beyond what it already does, so I think I'll just revert to more human-readable messages. IMO the UX is definitely more important than a currently unimplemented potential future machine-readable mechanism.
Concerns regarding the output format change
Continuing the work started in