Auto merge of #2480 - alexcrichton:ui, r=alexcrichton

Tweak UI for warnings and errors Right now Cargo prints out errors justified like all other status messages, but this can look odd without coloration: ``` error some error message ``` Instead, this commit changes both warnings and errors to use the same style of: ``` error: some error message warning: some warning message ``` Additionally, warnings now only print out "warning" in yellow instead of the entire message (like errors do)
rust-lang · Mar 17, 2016 · 834b834 · 834b834
2 parents c5360c4 + 8b35fbd
commit 834b834
Show file tree

Hide file tree

Showing 10 changed files with 35 additions and 22 deletions.
diff --git a/src/cargo/core/shell.rs b/src/cargo/core/shell.rs
@@ -73,7 +73,7 @@ impl MultiShell {
     {
         match self.verbosity {
             Quiet => Ok(()),
-            _ => self.out().say_status(status, message, GREEN)
+            _ => self.out().say_status(status, message, GREEN, true)
         }
     }
 
@@ -95,12 +95,12 @@ impl MultiShell {
         }
     }
 
-    pub fn error<T: ToString>(&mut self, message: T) -> CargoResult<()> {
-        self.err().say_status("error", message.to_string(), RED)
+    pub fn error<T: fmt::Display>(&mut self, message: T) -> CargoResult<()> {
+        self.err().say_status("error:", message, RED, false)
     }
 
-    pub fn warn<T: ToString>(&mut self, message: T) -> CargoResult<()> {
-        self.err().say(message, YELLOW)
+    pub fn warn<T: fmt::Display>(&mut self, message: T) -> CargoResult<()> {
+        self.err().say_status("warning:", message, YELLOW, false)
     }
 
     pub fn set_verbosity(&mut self, verbose: bool, quiet: bool) -> CargoResult<()> {
@@ -179,14 +179,22 @@ impl Shell {
         Ok(())
     }
 
-    pub fn say_status<T, U>(&mut self, status: T, message: U, color: Color)
+    pub fn say_status<T, U>(&mut self,
+                            status: T,
+                            message: U,
+                            color: Color,
+                            justified: bool)
                             -> CargoResult<()>
         where T: fmt::Display, U: fmt::Display
     {
         try!(self.reset());
         if color != BLACK { try!(self.fg(color)); }
         if self.supports_attr(Attr::Bold) { try!(self.attr(Attr::Bold)); }
-        try!(write!(self, "{:>12}", status.to_string()));
+        if justified {
+            try!(write!(self, "{:>12}", status.to_string()));
+        } else {
+            try!(write!(self, "{}", status));
+        }
         try!(self.reset());
         try!(write!(self, " {}\n", message));
         try!(self.flush());

diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs
@@ -183,12 +183,12 @@ pub fn shell(verbosity: Verbosity, color_config: ColorConfig) -> MultiShell {
 // and for others, e.g. docopt version info, print to stdout.
 fn output(err: String, shell: &mut MultiShell, fatal: bool) {
     let (std_shell, color, message) = if fatal {
-        (shell.err(), RED, Some("error"))
+        (shell.err(), RED, Some("error:"))
     } else {
         (shell.out(), BLACK, None)
     };
     let _ = match message{
-        Some(text) => std_shell.say_status(text, err.to_string(), color),
+        Some(text) => std_shell.say_status(text, err.to_string(), color, false),
         None => std_shell.say(err, color)
     };
 }
@@ -201,7 +201,8 @@ pub fn handle_error(err: CliError, shell: &mut MultiShell) {
 
     let hide = unknown && shell.get_verbose() != Verbose;
     if hide {
-        let _ = shell.err().say_status("error", "An unknown error occurred", RED);
+        let _ = shell.err().say_status("error:", "An unknown error occurred",
+                                       RED, false);
     } else {
         output(error.to_string(), shell, fatal);
     }

diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs
@@ -93,7 +93,7 @@ fn check_metadata(pkg: &Package, config: &Config) -> CargoResult<()> {
         things.push_str(&missing.last().unwrap());
 
         try!(config.shell().warn(
-            &format!("warning: manifest has no {things}. \
+            &format!("manifest has no {things}. \
                     See http://doc.crates.io/manifest.html#package-metadata for more info.",
                     things = things)))
     }

diff --git a/src/cargo/util/flock.rs b/src/cargo/util/flock.rs
@@ -250,7 +250,7 @@ fn acquire(config: &Config,
         }
     }
     let msg = format!("waiting for file lock on {}", msg);
-    try!(config.shell().err().say_status("Blocking", &msg, CYAN));
+    try!(config.shell().err().say_status("Blocking", &msg, CYAN, true));
 
     block().chain_error(|| {
         human(format!("failed to lock file: {}", path.display()))

diff --git a/src/cargo/util/toml.rs b/src/cargo/util/toml.rs
@@ -568,8 +568,8 @@ impl TomlManifest {
                                          profiles,
                                          publish);
         if project.license_file.is_some() && project.license.is_some() {
-            manifest.add_warning(format!("warning: only one of `license` or \
-                                                   `license-file` is necessary"));
+            manifest.add_warning(format!("only one of `license` or \
+                                          `license-file` is necessary"));
         }
         for warning in warnings {
             manifest.add_warning(warning.clone());
@@ -680,7 +680,7 @@ fn process_dependencies(cx: &mut Context,
 
         if details.version.is_none() && details.path.is_none() &&
            details.git.is_none() {
-            cx.warnings.push(format!("warning: dependency ({}) specified \
+            cx.warnings.push(format!("dependency ({}) specified \
                                       without providing a local path, Git \
                                       repository, or version to use. This will \
                                       be considered an error in future \
@@ -839,7 +839,7 @@ fn normalize(lib: &Option<TomlLibTarget>,
                 kinds.iter().filter_map(|s| {
                     let kind = LibKind::from_str(s);
                     if let Err(ref error) = kind {
-                        warnings.push(format!("warning: {}", error))
+                        warnings.push(error.to_string());
                     }
                     kind.ok()
                 }).collect()

diff --git a/tests/support/mod.rs b/tests/support/mod.rs
@@ -659,7 +659,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 ERROR:       &'static str = "error:";
 pub static DOCUMENTING: &'static str = " Documenting";
 pub static FRESH:       &'static str = "       Fresh";
 pub static UPDATING:    &'static str = "    Updating";

diff --git a/tests/test_bad_config.rs b/tests/test_bad_config.rs
@@ -398,7 +398,7 @@ test!(unused_keys {
 
     assert_that(foo.cargo_process("build"),
                 execs().with_status(0).with_stderr("\
-unused manifest key: target.foo.bar
+warning: unused manifest key: target.foo.bar
 "));
 });
 

diff --git a/tests/test_cargo_compile.rs b/tests/test_cargo_compile.rs
@@ -811,7 +811,9 @@ test!(unused_keys {
         "#);
     assert_that(p.cargo_process("build"),
                 execs().with_status(0)
-                       .with_stderr("unused manifest key: project.bulid\n"));
+                       .with_stderr("\
+warning: unused manifest key: project.bulid
+"));
 
     let mut p = project("bar");
     p = p
@@ -832,7 +834,9 @@ test!(unused_keys {
         "#);
     assert_that(p.cargo_process("build"),
                 execs().with_status(0)
-                       .with_stderr("unused manifest key: lib.build\n"));
+                       .with_stderr("\
+warning: unused manifest key: lib.build
+"));
 });
 
 test!(self_dependency {

diff --git a/tests/test_cargo_concurrent.rs b/tests/test_cargo_concurrent.rs
@@ -87,7 +87,7 @@ test!(one_install_should_be_bad {
 {error} binary `foo[..]` already exists in destination as part of `[..]`
 ", error = ERROR)));
     assert_that(good, execs().with_status(0).with_stderr("\
-be sure to add `[..]` to your PATH [..]
+warning: be sure to add `[..]` to your PATH [..]
 "));
 
     assert_that(cargo_home(), has_installed_exe("foo"));

diff --git a/tests/test_cargo_install.rs b/tests/test_cargo_install.rs
@@ -579,7 +579,7 @@ test!(do_not_rebuilds_on_local_install {
                 execs().with_status(0).with_stdout(&format!("\
 {installing} [..]
 ", installing = INSTALLING)).with_stderr("\
-be sure to add `[..]` to your PATH to be able to run the installed binaries
+warning: be sure to add `[..]` to your PATH to be able to run the installed binaries
 "));
 
     assert!(p.build_dir().c_exists());