-
Notifications
You must be signed in to change notification settings - Fork 70
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
Allow hiding warnings in Scarb output #1689
base: main
Are you sure you want to change the base?
Conversation
/// Avoid printing warnings to standard output. | ||
/// | ||
/// String representation: `no-warnings`. | ||
NoWarnings, |
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.
This can be
NoWarnings, | |
Error, |
Indicating that it will print errors but not warnings
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.
That was my initial thought too, but we have some outputs that aren’t warnings or errors, like:
ws.config()ui().print(Status::new("Packaging", &pkg_id.to_string()));
I figured we probably want to keep these messages and only mute warnings, so NoWarnings
seemed more fitting.
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.
Yes, I would still use ERROR
as it's more customary
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 it's more customary? It might make sense as environment variable setting, but, unlike --quiet
, --verbose
and --no-warnings
, --error
as a flag imo doesn't seem like it is directly related to verbosity, and reading docs will be required to understand what it actually does.
#[test_case(1, 0, false, Verbosity::Quiet, tracing_core::LevelFilter::OFF)] | ||
#[test_case(0, 0, false, Verbosity::Normal, tracing_core::LevelFilter::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.
Why we do not need to change this tests, if we do:
impl From<VerbositySpec> for Verbosity {
fn from(spec: VerbositySpec) -> Self {
match spec.integer_verbosity() {
v if v < -1 => Verbosity::Quiet,
-1 => Verbosity::NoWarnings,
Shouldn't quiet=1, verbose=0
be the new one then? 🤔
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'm not sure I understand, quiet=1, verbose=0
shouldn't produce any different verbosity level than before? That's not something this PR aims to change 🤔
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.
But you are changing impl From<VerbositySpec> for Verbosity
which is used to parse it, right?
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 am, but only to create conditions for setting NoWarnings
verbosity level, without affecting impact of --quiet
and --verbose
flags on settings their respectful verbosity level.
utils/scarb-ui/src/args/verbosity.rs
Outdated
.map(Verbosity::level_value) | ||
.unwrap_or(int_level) | ||
let int_level = if self.no_warnings { | ||
-1 |
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.
Why check twice? 🤔
Can we do
if self.no_warnings {
return -1;
}
at the beginning of this function, and avoid changing anything else?
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.
Yep, I initially added the check at the beginning of the function, but later thought setting int_level
first is just more in line with existing approach. Removed the extra check 👍🏻
7afde01
to
d82fd9a
Compare
Add
no-warnings
verbosity option, available as--no-warnings
flag or env var settingCloses #1660