diff mbox

[v3] Add documentation on clippy, fix clippy warnings

Message ID 20170105055336.30018-1-andrew.donnellan@au1.ibm.com
State Accepted
Headers show

Commit Message

Andrew Donnellan Jan. 5, 2017, 5:53 a.m. UTC
clippy[0] is the de facto standard linter for Rust. Add documentation
explaining how to run clippy on a nightly toolchain. Fix the warnings it
gives us and add a few exceptions where reasonable.

[0] https://github.com/Manishearth/rust-clippy

Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
---
v2->v3:
* get rid of the feature stuff, just tell people to use cargo +nightly 
clippy

---

 CONTRIBUTING.md  |  7 ++++++-
 src/git.rs       | 10 +++++-----
 src/jenkins.rs   | 15 +++++++--------
 src/main.rs      | 36 +++++++++++++++++-------------------
 src/patchwork.rs |  2 ++
 src/settings.rs  |  2 +-
 6 files changed, 38 insertions(+), 34 deletions(-)

Comments

Andrew Donnellan Jan. 6, 2017, 1:28 a.m. UTC | #1
On 05/01/17 16:53, Andrew Donnellan wrote:
> clippy[0] is the de facto standard linter for Rust. Add documentation
> explaining how to run clippy on a nightly toolchain. Fix the warnings it
> gives us and add a few exceptions where reasonable.
>
> [0] https://github.com/Manishearth/rust-clippy
>
> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

Applied to master: e16812929eace50e676f73671442b9552115cf58
diff mbox

Patch

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index d067da9..8d04072 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -28,6 +28,11 @@  We like to follow the [Rust Guidelines](https://aturon.github.io/)
 where possible - patches to fix existing non-compliant code are
 welcome!
 
+If you're using a nightly Rust toolchain, you can use the
+[clippy](https://github.com/Manishearth/rust-clippy) linter: `cargo
++nightly install clippy` to install, and `cargo +nightly clippy` to
+run.
+
 When your patch involves creating a new file, where possible please
 use a header along the lines of:
 
@@ -46,4 +51,4 @@  use a header along the lines of:
 #
 # [FILENAME] - [DESCRIPTION]
 #
-```
\ No newline at end of file
+```
diff --git a/src/git.rs b/src/git.rs
index 679d387..9be867f 100644
--- a/src/git.rs
+++ b/src/git.rs
@@ -26,7 +26,7 @@  pub static GIT_REF_BASE: &'static str = "refs/heads";
 pub fn get_latest_commit(repo: &Repository) -> Commit {
     let head = repo.head().unwrap();
     let oid = head.target().unwrap();
-    return repo.find_commit(oid).unwrap();
+    repo.find_commit(oid).unwrap()
 }
 
 pub fn push_to_remote(remote: &mut Remote, branch: &str,
@@ -48,9 +48,9 @@  pub fn pull(repo: &Repository) -> Result<Output, &'static str> {
 
     if output.status.success() {
         debug!("Pull: {}", String::from_utf8(output.clone().stdout).unwrap());
-        return Ok(output);
+        Ok(output)
     } else {
-        return Err("Error: couldn't pull changes");
+        Err("Error: couldn't pull changes")
     }
 
 }
@@ -77,11 +77,11 @@  pub fn apply_patch(repo: &Repository, path: &Path)
     if output.status.success() {
         debug!("Patch applied with text {}",
                  String::from_utf8(output.clone().stdout).unwrap());
-        return Ok(output);
+        Ok(output)
     } else {
         Command::new("git").arg("am").arg("--abort")
             .current_dir(&workdir).output().unwrap();
-        return Err("Patch did not apply successfully");
+        Err("Patch did not apply successfully")
     }
 }
 
diff --git a/src/jenkins.rs b/src/jenkins.rs
index 6b92a85..1c63e75 100644
--- a/src/jenkins.rs
+++ b/src/jenkins.rs
@@ -73,6 +73,7 @@  impl CIBackend for JenkinsBackend {
     }
 }
 
+#[derive(Eq, PartialEq)]
 pub enum JenkinsBuildStatus {
     Running,
     Done,
@@ -131,20 +132,18 @@  impl JenkinsBackend {
     }
 
     pub fn get_build_status(&self, build_url: &str) -> JenkinsBuildStatus {
-        match self.get_api_json_object(build_url).get("building").unwrap().as_boolean().unwrap() {
-            true => JenkinsBuildStatus::Running,
-            false => JenkinsBuildStatus::Done,
+        if self.get_api_json_object(build_url)["building"].as_boolean().unwrap() {
+            JenkinsBuildStatus::Running
+        } else {
+            JenkinsBuildStatus::Done
         }
     }
 
     pub fn wait_build(&self, build_url: &str) -> JenkinsBuildStatus {
         // TODO: Implement a timeout?
-        loop {
-            match self.get_build_status(&build_url) {
-                JenkinsBuildStatus::Done => return JenkinsBuildStatus::Done,
-                _ => { },
-            }
+        while self.get_build_status(build_url) != JenkinsBuildStatus::Done {
             sleep(Duration::from_millis(JENKINS_POLLING_INTERVAL));
         }
+        JenkinsBuildStatus::Done
     }
 }
diff --git a/src/main.rs b/src/main.rs
index 426bfdf..e4d2389 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -99,7 +99,7 @@  fn run_tests(settings: &Config, client: Arc<Client>, project: &Project, tag: &st
         token: settings.jenkins.token.clone(),
     };
     let project = project.clone();
-    for job_params in project.jobs.iter() {
+    for job_params in &project.jobs {
         let job_name = job_params.get("job").unwrap();
         let mut jenkins_params = Vec::<(&str, &str)>::new();
         for (param_name, param_value) in job_params.iter() {
@@ -107,21 +107,21 @@  fn run_tests(settings: &Config, client: Arc<Client>, project: &Project, tag: &st
             match param_name.as_ref() {
                 // TODO: Validate special parameter names in config at start of program
                 "job" => { },
-                "remote" => jenkins_params.push((&param_value, &project.remote_uri)),
-                "branch" => jenkins_params.push((&param_value, &tag)),
-                _ => jenkins_params.push((&param_name, &param_value)),
+                "remote" => jenkins_params.push((param_value, &project.remote_uri)),
+                "branch" => jenkins_params.push((param_value, tag)),
+                _ => jenkins_params.push((param_name, param_value)),
             }
         }
         info!("Starting job: {}", &job_name);
-        let res = jenkins.start_test(&job_name, jenkins_params)
+        let res = jenkins.start_test(job_name, jenkins_params)
             .unwrap_or_else(|err| panic!("Starting Jenkins test failed: {}", err));
         debug!("{:?}", &res);
         let build_url_real;
         loop {
             let build_url = jenkins.get_build_url(&res);
-            match build_url {
-                Some(url) => { build_url_real = url; break; },
-                None => { },
+            if let Some(url) = build_url {
+                build_url_real = url;
+                break;
             }
         }
         debug!("Build URL: {}", build_url_real);
@@ -150,7 +150,7 @@  fn test_patch(settings: &Config, client: &Arc<Client>, project: &Project, path:
 
     let mut push_callbacks = RemoteCallbacks::new();
     push_callbacks.credentials(|_, _, _| {
-        return Cred::ssh_key_from_agent("git");
+        Cred::ssh_key_from_agent("git")
     });
 
     let mut push_opts = PushOptions::new();
@@ -175,12 +175,10 @@  fn test_patch(settings: &Config, client: &Arc<Client>, project: &Project, path:
             .unwrap_or_else(|err| panic!("Couldn't checkout HEAD: {}", err));
         debug!("Repo is now at head {}", repo.head().unwrap().name().unwrap());
 
-        let output = git::apply_patch(&repo, &path);
+        let output = git::apply_patch(&repo, path);
 
-        match output {
-            Ok(_) => git::push_to_remote(
-                &mut remote, &tag, &mut push_opts).unwrap(),
-            _ => {}
+        if output.is_ok() {
+            git::push_to_remote(&mut remote, &tag, &mut push_opts).unwrap();
         }
 
         git::checkout_branch(&repo, &branch_name);
@@ -219,8 +217,7 @@  fn test_patch(settings: &Config, client: &Arc<Client>, project: &Project, path:
 
         // We've set up a remote branch, time to kick off tests
         let test = thread::Builder::new().name(tag.to_string()).spawn(move || {
-            return run_tests(&settings_clone, client, &project, &tag,
-                             &branch_name);
+            run_tests(&settings_clone, client, &project, &tag, &branch_name)
         }).unwrap();
         results.append(&mut test.join().unwrap());
 
@@ -238,6 +235,7 @@  fn test_patch(settings: &Config, client: &Arc<Client>, project: &Project, path:
     results
 }
 
+#[cfg_attr(feature="cargo-clippy", allow(cyclomatic_complexity))]
 fn main() {
     let mut log_builder = LogBuilder::new();
     // By default, log at the "info" level for every module
@@ -292,7 +290,7 @@  fn main() {
         match settings.projects.get(&args.flag_project) {
             None => panic!("Couldn't find project {}", args.flag_project),
             Some(project) => {
-                test_patch(&settings, &client, &project, &patch);
+                test_patch(&settings, &client, project, patch);
             }
         }
 
@@ -306,7 +304,7 @@  fn main() {
             None => panic!("Couldn't find project {}", &series.project.linkname),
             Some(project) => {
                 let patch = patchwork.get_patch(&series);
-                test_patch(&settings, &client, &project, &patch);
+                test_patch(&settings, &client, project, &patch);
             }
         }
 
@@ -342,7 +340,7 @@  fn main() {
                 },
                 Some(project) => {
                     let patch = patchwork.get_patch(&series);
-                    let results = test_patch(&settings, &client, &project, &patch);
+                    let results = test_patch(&settings, &client, project, &patch);
                     // Delete the temporary directory with the patch in it
                     fs::remove_dir_all(patch.parent().unwrap()).unwrap_or_else(
                         |err| error!("Couldn't delete temp directory: {}", err));
diff --git a/src/patchwork.rs b/src/patchwork.rs
index fa36da9..a6f761a 100644
--- a/src/patchwork.rs
+++ b/src/patchwork.rs
@@ -116,6 +116,7 @@  pub struct PatchworkServer {
 }
 
 impl PatchworkServer {
+    #[cfg_attr(feature="cargo-clippy", allow(ptr_arg))]
     pub fn new(url: &String, client: &std::sync::Arc<Client>) -> PatchworkServer {
         let mut headers = Headers::new();
         headers.set(Accept(vec![qitem(Mime(TopLevel::Application,
@@ -133,6 +134,7 @@  impl PatchworkServer {
         }
     }
 
+    #[cfg_attr(feature="cargo-clippy", allow(ptr_arg))]
     pub fn set_authentication(&mut self, username: &String, password: &Option<String>) {
         self.headers.set(Authorization(Basic {
             username: username.clone(),
diff --git a/src/settings.rs b/src/settings.rs
index a4a5614..4e91244 100644
--- a/src/settings.rs
+++ b/src/settings.rs
@@ -59,7 +59,7 @@  pub struct Project {
 
 impl Project {
     pub fn get_repo(&self) -> Result<Repository, Error> {
-        return Repository::open(&self.repository);
+        Repository::open(&self.repository)
     }
 }