-
Notifications
You must be signed in to change notification settings - Fork 71
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?
Changes from all commits
5dff4c5
d7cd6ee
f2a8073
eb2f726
d82fd9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,6 +110,22 @@ fn dry_run() { | |
.run(); | ||
} | ||
|
||
#[test] | ||
fn dry_run_without_warnings() { | ||
ManifestEditHarness::offline() | ||
.args(["add", "--dry-run", "--no-warnings", "[email protected]"]) | ||
.input(indoc! {r#" | ||
[package] | ||
name = "hello" | ||
version = "1.0.0" | ||
|
||
[dependencies] | ||
bar = "1.0.0" | ||
"#}) | ||
.stdout_matches("") | ||
.run(); | ||
} | ||
|
||
#[test] | ||
fn path() { | ||
let t = TempDir::new().unwrap(); | ||
|
DelevoXDG marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -14,6 +14,10 @@ pub enum Verbosity { | |||||
/// | ||||||
/// String representation: `quiet`. | ||||||
Quiet, | ||||||
/// Avoid printing warnings to standard output. | ||||||
/// | ||||||
/// String representation: `no-warnings`. | ||||||
NoWarnings, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be
Suggested change
Indicating that it will print errors but not warnings There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I would still use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
/// Default verbosity level. | ||||||
/// | ||||||
/// String representation: `normal`. | ||||||
|
@@ -29,6 +33,7 @@ impl Display for Verbosity { | |||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||||||
match self { | ||||||
Self::Quiet => write!(f, "quiet"), | ||||||
Self::NoWarnings => write!(f, "no-warnings"), | ||||||
Self::Normal => write!(f, "normal"), | ||||||
Self::Verbose => write!(f, "verbose"), | ||||||
} | ||||||
|
@@ -41,6 +46,7 @@ impl FromStr for Verbosity { | |||||
fn from_str(s: &str) -> Result<Self> { | ||||||
match s { | ||||||
"quiet" => Ok(Verbosity::Quiet), | ||||||
"no-warnings" => Ok(Verbosity::NoWarnings), | ||||||
"normal" => Ok(Verbosity::Normal), | ||||||
"verbose" => Ok(Verbosity::Verbose), | ||||||
"" => bail!("empty string cannot be used as verbosity level"), | ||||||
|
@@ -69,14 +75,19 @@ mod tests { | |||||
#[test] | ||||||
fn verbosity_ord() { | ||||||
use Verbosity::*; | ||||||
assert!(Quiet < Normal); | ||||||
assert!(Quiet < NoWarnings); | ||||||
assert!(NoWarnings < Normal); | ||||||
assert!(Normal < Verbose); | ||||||
} | ||||||
|
||||||
#[test] | ||||||
fn verbosity_from_str() { | ||||||
use Verbosity::*; | ||||||
assert_eq!(Quiet.to_string().parse::<Verbosity>().unwrap(), Quiet); | ||||||
assert_eq!( | ||||||
NoWarnings.to_string().parse::<Verbosity>().unwrap(), | ||||||
NoWarnings | ||||||
); | ||||||
assert_eq!(Normal.to_string().parse::<Verbosity>().unwrap(), Normal); | ||||||
assert_eq!(Verbose.to_string().parse::<Verbosity>().unwrap(), Verbose); | ||||||
} | ||||||
|
@@ -88,6 +99,8 @@ mod tests { | |||||
assert_eq!(Verbosity::from_env_var("SOME_ENV_VAR").unwrap(), Quiet); | ||||||
env::set_var("SOME_ENV_VAR", "verbose"); | ||||||
assert_eq!(Verbosity::from_env_var("SOME_ENV_VAR").unwrap(), Verbose); | ||||||
env::set_var("SOME_ENV_VAR", "no-warnings"); | ||||||
assert_eq!(Verbosity::from_env_var("SOME_ENV_VAR").unwrap(), NoWarnings); | ||||||
assert!(Verbosity::from_env_var("SOME_ENV_VAR_THAT_DOESNT_EXIST").is_err()); | ||||||
} | ||||||
} |
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:
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.