Skip to content

Commit

Permalink
Auto merge of #2421 - sbeckeriv:decolor-messages-426, r=alexcrichton
Browse files Browse the repository at this point in the history 
Dull the errors

This resolves #426

Dearest Reviewer,

I have updated the error messages to use say_status at the shell level. I have also changed say_status to print the message in bold. I do think it looks nice but it does have the side effect of making some seemingly unrelated text bold. I do think it looks better bold but it is also very easy to revert. I have included examples of both.

Thank you,
Becker

Bold: Note the usage is bold.
<img width="1072" alt="screen shot 2016-02-27 at 10 49 05 am" src="https://cloud.githubusercontent.com/assets/12170/13374778/0efd54ec-dd43-11e5-9f02-f0224608132a.png">

No bold:
<img width="885" alt="screen shot 2016-02-27 at 10 46 35 am" src="https://cloud.githubusercontent.com/assets/12170/13374775/fa3a6612-dd42-11e5-9c09-8f23506f5f0c.png">

Both rendered on a mac with iterm 2.9.0
  • Loading branch information
@bors
bors committed Mar 12, 2016
2 parents 082a7dd + 1c43987 commit 626eeb7
Show file tree
Hide file tree
Showing 30 changed files with 513 additions and 396 deletions.
2 changes: 1 addition & 1 deletion src/bin/bench.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
None => Ok(None),
Some(err) => {
Err(match err.exit.as_ref().and_then(|e| e.code()) {
Some(i) => CliError::new("", i),
Some(i) => CliError::new("bench failed", i),
None => CliError::from_error(Human(err), 101)
})
}
Expand Down
2 changes: 1 addition & 1 deletion src/bin/test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
None => Ok(None),
Some(err) => {
Err(match err.exit.as_ref().and_then(|e| e.code()) {
Some(i) => CliError::new("", i),
Some(i) => CliError::new("test failed", i),
None => CliError::from_error(Human(err), 101)
})
}
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/shell.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ impl MultiShell {
}

pub fn error<T: ToString>(&mut self, message: T) -> CargoResult<()> {
self.err().say(message, RED)
self.err().say_status("error", message.to_string(), RED)
}

pub fn warn<T: ToString>(&mut self, message: T) -> CargoResult<()> {
Expand Down
14 changes: 10 additions & 4 deletions src/cargo/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,9 +181,15 @@ pub fn shell(verbosity: Verbosity, color_config: ColorConfig) -> MultiShell {
// For fatal errors, print to stderr;
// and for others, e.g. docopt version info, print to stdout.
fn output(err: String, shell: &mut MultiShell, fatal: bool) {
let std_shell = if fatal {shell.err()} else {shell.out()};
let color = if fatal {RED} else {BLACK};
let _ = std_shell.say(err, color);
let (std_shell, color, message) = if fatal {
(shell.err(), RED, Some("error"))
} else {
(shell.out(), BLACK, None)
};
let _ = match message{
Some(text) => std_shell.say_status(text, err.to_string(), color),
None => std_shell.say(err, color)
};
}

pub fn handle_error(err: CliError, shell: &mut MultiShell) {
Expand All @@ -194,7 +200,7 @@ pub fn handle_error(err: CliError, shell: &mut MultiShell) {

let hide = unknown && shell.get_verbose() != Verbose;
if hide {
let _ = shell.err().say("An unknown error occurred", RED);
let _ = shell.err().say_status("error", "An unknown error occurred", RED);
} else {
output(error.to_string(), shell, fatal);
}
Expand Down
1 change: 1 addition & 0 deletions tests/support/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,7 @@ pub fn path2url(p: PathBuf) -> Url {

pub static RUNNING: &'static str = " Running";
pub static COMPILING: &'static str = " Compiling";
pub static ERROR: &'static str = " error";
pub static DOCUMENTING: &'static str = " Documenting";
pub static FRESH: &'static str = " Fresh";
pub static UPDATING: &'static str = " Updating";
Expand Down
93 changes: 53 additions & 40 deletions tests/test_bad_config.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use support::{project, execs};
use support::{project, execs, ERROR};
use support::registry::Package;
use hamcrest::assert_that;

Expand All @@ -19,9 +19,11 @@ test!(bad1 {
"#);
assert_that(foo.cargo_process("build").arg("-v")
.arg("--target=nonexistent-target"),
execs().with_status(101).with_stderr("\
expected table for configuration key `target.nonexistent-target`, but found string in [..]config
"));
execs().with_status(101).with_stderr(&format!("\
{error} expected table for configuration key `target.nonexistent-target`, \
but found string in [..]config
",
error = ERROR)));
});

test!(bad2 {
Expand All @@ -38,8 +40,8 @@ test!(bad2 {
proxy = 3.0
"#);
assert_that(foo.cargo_process("publish").arg("-v"),
execs().with_status(101).with_stderr("\
Couldn't load Cargo configuration
execs().with_status(101).with_stderr(&format!("\
{error} Couldn't load Cargo configuration
Caused by:
failed to load TOML configuration from `[..]config`
Expand All @@ -52,7 +54,7 @@ Caused by:
Caused by:
found TOML configuration value of unknown type `float`
"));
", error = ERROR)));
});

test!(bad3 {
Expand All @@ -69,10 +71,11 @@ test!(bad3 {
proxy = true
"#);
assert_that(foo.cargo_process("publish").arg("-v"),
execs().with_status(101).with_stderr("\
invalid configuration for key `http.proxy`
execs().with_status(101).with_stderr(&format!("\
{error} invalid configuration for key `http.proxy`
expected a string, but found a boolean in [..]config
"));
",
error = ERROR)));
});

test!(bad4 {
Expand All @@ -82,13 +85,14 @@ test!(bad4 {
name = false
"#);
assert_that(foo.cargo_process("new").arg("-v").arg("foo"),
execs().with_status(101).with_stderr("\
Failed to create project `foo` at `[..]`
execs().with_status(101).with_stderr(&format!("\
{error} Failed to create project `foo` at `[..]`
Caused by:
invalid configuration for key `cargo-new.name`
expected a string, but found a boolean in [..]config
"));
",
error = ERROR)));
});

test!(bad5 {
Expand All @@ -102,8 +106,8 @@ test!(bad5 {
foo.build();
assert_that(foo.cargo("new")
.arg("-v").arg("foo").cwd(&foo.root().join("foo")),
execs().with_status(101).with_stderr("\
Couldn't load Cargo configuration
execs().with_status(101).with_stderr(&format!("\
{error} Couldn't load Cargo configuration
Caused by:
failed to merge key `foo` between files:
Expand All @@ -112,7 +116,8 @@ Caused by:
Caused by:
expected integer, but found string
"));
",
error = ERROR)));
});

test!(bad_cargo_config_jobs {
Expand All @@ -129,9 +134,10 @@ test!(bad_cargo_config_jobs {
jobs = -1
"#);
assert_that(foo.cargo_process("build").arg("-v"),
execs().with_status(101).with_stderr("\
build.jobs must be positive, but found -1 in [..]
"));
execs().with_status(101).with_stderr(&format!("\
{error} build.jobs must be positive, but found -1 in [..]
",
error = ERROR)));
});

test!(default_cargo_config_jobs {
Expand Down Expand Up @@ -183,8 +189,8 @@ test!(invalid_global_config {
.file("src/lib.rs", "");

assert_that(foo.cargo_process("build").arg("-v"),
execs().with_status(101).with_stderr("\
Couldn't load Cargo configuration
execs().with_status(101).with_stderr(&format!("\
{error} Couldn't load Cargo configuration
Caused by:
could not parse TOML configuration in `[..]config`
Expand All @@ -193,7 +199,8 @@ Caused by:
could not parse input as TOML
[..]config:1:2 expected `=`, but found eof
"));
",
error = ERROR)));
});

test!(bad_cargo_lock {
Expand All @@ -208,12 +215,13 @@ test!(bad_cargo_lock {
.file("src/lib.rs", "");

assert_that(foo.cargo_process("build").arg("-v"),
execs().with_status(101).with_stderr("\
failed to parse lock file at: [..]Cargo.lock
execs().with_status(101).with_stderr(&format!("\
{error} failed to parse lock file at: [..]Cargo.lock
Caused by:
expected a section for the key `root`
"));
",
error = ERROR)));
});

test!(bad_git_dependency {
Expand All @@ -230,15 +238,16 @@ test!(bad_git_dependency {
.file("src/lib.rs", "");

assert_that(foo.cargo_process("build").arg("-v"),
execs().with_status(101).with_stderr("\
Unable to update file:///
execs().with_status(101).with_stderr(&format!("\
{error} Unable to update file:///
Caused by:
failed to clone into: [..]
Caused by:
[[..]] 'file:///' is not a valid local file URI
"));
",
error = ERROR)));
});

test!(bad_crate_type {
Expand Down Expand Up @@ -276,14 +285,15 @@ test!(malformed_override {
.file("src/lib.rs", "");

assert_that(foo.cargo_process("build"),
execs().with_status(101).with_stderr("\
failed to parse manifest at `[..]`
execs().with_status(101).with_stderr(&format!("\
{error} failed to parse manifest at `[..]`
Caused by:
could not parse input as TOML
Cargo.toml:[..]
"));
",
error = ERROR)));
});

test!(duplicate_binary_names {
Expand All @@ -306,12 +316,13 @@ test!(duplicate_binary_names {
.file("b.rs", r#"fn main() -> () {}"#);

assert_that(foo.cargo_process("build"),
execs().with_status(101).with_stderr("\
failed to parse manifest at `[..]`
execs().with_status(101).with_stderr(&format!("\
{error} failed to parse manifest at `[..]`
Caused by:
found duplicate binary name e, but all binary targets must have a unique name
"));
",
error = ERROR)));
});

test!(duplicate_example_names {
Expand All @@ -334,12 +345,13 @@ test!(duplicate_example_names {
.file("examples/ex2.rs", r#"fn main () -> () {}"#);

assert_that(foo.cargo_process("build").arg("--example").arg("ex"),
execs().with_status(101).with_stderr("\
failed to parse manifest at `[..]`
execs().with_status(101).with_stderr(&format!("\
{error} failed to parse manifest at `[..]`
Caused by:
found duplicate example name ex, but all binary targets must have a unique name
"));
",
error = ERROR)));
});

test!(duplicate_bench_names {
Expand All @@ -362,12 +374,13 @@ test!(duplicate_bench_names {
.file("benches/ex2.rs", r#"fn main () {}"#);

assert_that(foo.cargo_process("bench"),
execs().with_status(101).with_stderr("\
failed to parse manifest at `[..]`
execs().with_status(101).with_stderr(&format!("\
{error} failed to parse manifest at `[..]`
Caused by:
found duplicate bench name ex, but all binary targets must have a unique name
"));
",
error = ERROR)));
});

test!(unused_keys {
Expand Down
9 changes: 6 additions & 3 deletions tests/test_bad_manifest_path.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use support::{project, execs, main_file, basic_bin_manifest};
use support::{project, execs, main_file, basic_bin_manifest, ERROR};
use hamcrest::{assert_that};

fn setup() {}
Expand All @@ -12,7 +12,9 @@ fn assert_not_a_cargo_toml(command: &str, manifest_path_argument: &str) {
.arg("--manifest-path").arg(manifest_path_argument)
.cwd(p.root().parent().unwrap()),
execs().with_status(101)
.with_stderr("the manifest-path must be a path to a Cargo.toml file"));
.with_stderr(&format!("{error} the manifest-path must be a path \
to a Cargo.toml file",
error = ERROR)));
}

#[allow(deprecated)] // connect => join in 1.3
Expand All @@ -26,7 +28,8 @@ fn assert_cargo_toml_doesnt_exist(command: &str, manifest_path_argument: &str) {
.cwd(p.root().parent().unwrap()),
execs().with_status(101)
.with_stderr(
format!("manifest path `{}` does not exist", expected_path)
format!("{error} manifest path `{}` does not exist",
expected_path, error = ERROR)
));
}

Expand Down
12 changes: 7 additions & 5 deletions tests/test_cargo.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use std::str;

use cargo_process;
use support::paths;
use support::{execs, project, mkdir_recursive, ProjectBuilder};
use support::{execs, project, mkdir_recursive, ProjectBuilder, ERROR};
use hamcrest::{assert_that};

fn setup() {
Expand Down Expand Up @@ -61,11 +61,12 @@ test!(find_closest_biuld_to_build {

assert_that(pr,
execs().with_status(101)
.with_stderr("no such subcommand
.with_stderr(&format!("{error} no such subcommand
<tab>Did you mean `build`?
"));
",
error = ERROR)));
});

// if a subcommand is more than 3 edit distance away, we don't make a suggestion
Expand All @@ -83,8 +84,9 @@ test!(find_closest_dont_correct_nonsense {

assert_that(pr,
execs().with_status(101)
.with_stderr("no such subcommand
"));
.with_stderr(&format!("{error} no such subcommand
",
error = ERROR)));
});

test!(override_cargo_home {
Expand Down
Loading

0 comments on commit 626eeb7

Please sign in to comment.