Updated several clippy_dev messages and types (PR suggestions)

xFrednet · flip1995 · flip1995 · commit 8e969cdcefef · 2021-06-25T11:17:00.000+02:00
Co-authored-by: Philipp Krones &lt;hello@philkrones.com&gt;
diff --git a/clippy_dev/src/setup/git_hook.rs b/clippy_dev/src/setup/git_hook.rs
@@ -10,7 +10,7 @@ const HOOK_SOURCE_FILE: &str = "util/etc/pre-commit.sh";
 const HOOK_TARGET_FILE: &str = ".git/hooks/pre-commit";
 
 pub fn install_hook(force_override: bool) {
-    if check_precondition(force_override).is_err() {
+    if !check_precondition(force_override) {
         return;
     }
 
@@ -25,52 +25,55 @@ pub fn install_hook(force_override: bool) {
     // include the `execute` permission.
     match fs::copy(HOOK_SOURCE_FILE, HOOK_TARGET_FILE) {
         Ok(_) => {
-            println!("note: the hook can be removed with `cargo dev remove git-hook`");
-            println!("Git hook successfully installed :)");
+            println!("info: the hook can be removed with `cargo dev remove git-hook`");
+            println!("git hook successfully installed");
         },
-        Err(err) => println!(
+        Err(err) => eprintln!(
             "error: unable to copy `{}` to `{}` ({})",
             HOOK_SOURCE_FILE, HOOK_TARGET_FILE, err
         ),
     }
 }
 
-fn check_precondition(force_override: bool) -> Result<(), ()> {
+fn check_precondition(force_override: bool) -> bool {
     // Make sure that we can find the git repository
     let git_path = Path::new(REPO_GIT_DIR);
     if !git_path.exists() || !git_path.is_dir() {
-        println!("error: clippy_dev was unable to find the `.git` directory");
-        return Err(());
+        eprintln!("error: clippy_dev was unable to find the `.git` directory");
+        return false;
     }
 
     // Make sure that we don't override an existing hook by accident
     let path = Path::new(HOOK_TARGET_FILE);
     if path.exists() {
-        if force_override || super::ask_yes_no_question("Do you want to override the existing pre-commit hook it?") {
+        if force_override {
             return delete_git_hook_file(path);
         }
-        return Err(());
+
+        eprintln!("error: there is already a pre-commit hook installed");
+        println!("info: use the `--force-override` flag to override the existing hook");
+        return false;
     }
 
-    Ok(())
+    true
 }
 
 pub fn remove_hook() {
     let path = Path::new(HOOK_TARGET_FILE);
     if path.exists() {
-        if delete_git_hook_file(path).is_ok() {
-            println!("Git hook successfully removed :)");
+        if delete_git_hook_file(path) {
+            println!("git hook successfully removed");
         }
     } else {
-        println!("No pre-commit hook was found. You're good to go :)");
+        println!("no pre-commit hook was found");
     }
 }
 
-fn delete_git_hook_file(path: &Path) -> Result<(), ()> {
-    if fs::remove_file(path).is_err() {
-        println!("error: unable to delete existing pre-commit git hook");
-        Err(())
+fn delete_git_hook_file(path: &Path) -> bool {
+    if let Err(err) = fs::remove_file(path) {
+        eprintln!("error: unable to delete existing pre-commit git hook ({})", err);
+        false
     } else {
-        Ok(())
+        true
     }
 }
diff --git a/clippy_dev/src/setup/intellij.rs b/clippy_dev/src/setup/intellij.rs
@@ -5,8 +5,8 @@ use std::path::{Path, PathBuf};
 
 // This module takes an absolute path to a rustc repo and alters the dependencies to point towards
 // the respective rustc subcrates instead of using extern crate xyz.
-// This allows rust analyzer to analyze rustc internals and show proper information inside clippy
-// code. See https://github.com/rust-analyzer/rust-analyzer/issues/3517 and https://github.com/rust-lang/rust-clippy/issues/5514 for details
+// This allows IntelliJ to analyze rustc internals and show proper information inside Clippy
+// code. See https://github.com/rust-lang/rust-clippy/issues/5514 for details
 
 const RUSTC_PATH_SECTION: &str = "[target.'cfg(NOT_A_PLATFORM)'.dependencies]";
 const DEPENDENCIES_SECTION: &str = "[dependencies]";
@@ -75,16 +75,16 @@ fn check_and_get_rustc_dir(rustc_path: &str) -> Result<PathBuf, ()> {
     }
 
     if !path.is_dir() {
-        eprintln!("error: the given path is a file and not a directory");
+        eprintln!("error: the given path is not a directory");
         return Err(());
     }
 
     Ok(path)
 }
 
 fn inject_deps_into_project(rustc_source_dir: &Path, project: &ClippyProjectInfo) -> Result<(), ()> {
-    let cargo_content = read_project_file(project.cargo_file, "Cargo.toml", project.name)?;
-    let lib_content = read_project_file(project.lib_rs_file, "lib.rs", project.name)?;
+    let cargo_content = read_project_file(project.cargo_file)?;
+    let lib_content = read_project_file(project.lib_rs_file)?;
 
     if inject_deps_into_manifest(rustc_source_dir, project.cargo_file, &cargo_content, &lib_content).is_err() {
         eprintln!(
@@ -100,23 +100,17 @@ fn inject_deps_into_project(rustc_source_dir: &Path, project: &ClippyProjectInfo
 /// `clippy_dev` expects to be executed in the root directory of Clippy. This function
 /// loads the given file or returns an error. Having it in this extra function ensures
 /// that the error message looks nice.
-fn read_project_file(file_path: &str, file_name: &str, project: &str) -> Result<String, ()> {
+fn read_project_file(file_path: &str) -> Result<String, ()> {
     let path = Path::new(file_path);
     if !path.exists() {
-        eprintln!(
-            "error: unable to find the `{}` file for the project {}",
-            file_name, project
-        );
+        eprintln!("error: unable to find the file `{}`", file_path);
         return Err(());
     }
 
     match fs::read_to_string(path) {
         Ok(content) => Ok(content),
         Err(err) => {
-            println!(
-                "error: the `{}` file for the project {} could not be read ({})",
-                file_name, project, err
-            );
+            eprintln!("error: the file `{}` could not be read ({})", file_path, err);
             Err(())
         },
     }
@@ -142,8 +136,8 @@ fn inject_deps_into_manifest(
         // only take dependencies starting with `rustc_`
         .filter(|line| line.starts_with("extern crate rustc_"))
         // we have something like "extern crate foo;", we only care about the "foo"
-        //              ↓          ↓
         // extern crate rustc_middle;
+        //              ^^^^^^^^^^^^
         .map(|s| &s[13..(s.len() - 1)]);
 
     let new_deps = extern_crates.map(|dep| {
@@ -180,23 +174,24 @@ fn inject_deps_into_manifest(
 
 pub fn remove_rustc_src() {
     for project in CLIPPY_PROJECTS {
-        // We don't care about the result here as we want to go through all
-        // dependencies either way. Any info and error message will be issued by
-        // the removal code itself.
-        let _ = remove_rustc_src_from_project(project);
+        remove_rustc_src_from_project(project);
     }
 }
 
-fn remove_rustc_src_from_project(project: &ClippyProjectInfo) -> Result<(), ()> {
-    let mut cargo_content = read_project_file(project.cargo_file, "Cargo.toml", project.name)?;
+fn remove_rustc_src_from_project(project: &ClippyProjectInfo) -> bool {
+    let mut cargo_content = if let Ok(content) = read_project_file(project.cargo_file) {
+        content
+    } else {
+        return false;
+    };
     let section_start = if let Some(section_start) = cargo_content.find(RUSTC_PATH_SECTION) {
         section_start
     } else {
         println!(
             "info: dependencies could not be found in `{}` for {}, skipping file",
             project.cargo_file, project.name
         );
-        return Ok(());
+        return true;
     };
 
     let end_point = if let Some(end_point) = cargo_content.find(DEPENDENCIES_SECTION) {
@@ -206,7 +201,7 @@ fn remove_rustc_src_from_project(project: &ClippyProjectInfo) -> Result<(), ()>
             "error: the end of the rustc dependencies section could not be found in `{}`",
             project.cargo_file
         );
-        return Err(());
+        return false;
     };
 
     cargo_content.replace_range(section_start..end_point, "");
@@ -215,14 +210,14 @@ fn remove_rustc_src_from_project(project: &ClippyProjectInfo) -> Result<(), ()>
         Ok(mut file) => {
             file.write_all(cargo_content.as_bytes()).unwrap();
             println!("info: successfully removed dependencies inside {}", project.cargo_file);
-            Ok(())
+            true
         },
         Err(err) => {
             eprintln!(
                 "error: unable to open file `{}` to remove rustc dependencies for {} ({})",
                 project.cargo_file, project.name, err
             );
-            Err(())
+            false
         },
     }
 }
diff --git a/clippy_dev/src/setup/mod.rs b/clippy_dev/src/setup/mod.rs
@@ -1,30 +1,2 @@
-use std::io::{self, Write};
 pub mod git_hook;
 pub mod intellij;
-
-/// This function will asked the user the given question and wait for user input
-/// either `true` for yes and `false` for no.
-fn ask_yes_no_question(question: &str) -> bool {
-    // This code was proudly stolen from rusts bootstrapping tool.
-
-    fn ask_with_result(question: &str) -> io::Result<bool> {
-        let mut input = String::new();
-        Ok(loop {
-            print!("{}: [y/N] ", question);
-            io::stdout().flush()?;
-            input.clear();
-            io::stdin().read_line(&mut input)?;
-            break match input.trim().to_lowercase().as_str() {
-                "y" | "yes" => true,
-                "n" | "no" | "" => false,
-                _ => {
-                    println!("error: unrecognized option '{}'", input.trim());
-                    println!("note: press Ctrl+C to exit");
-                    continue;
-                },
-            };
-        })
-    }
-
-    ask_with_result(question).unwrap_or_default()
-}
