[snowpatch] [PATCH v3] Add documentation on clippy, fix clippy warnings
Andrew Donnellan
andrew.donnellan at au1.ibm.com
Thu Jan 5 16:53:36 AEDT 2017
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 at 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(-)
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((¶m_value, &project.remote_uri)),
- "branch" => jenkins_params.push((¶m_value, &tag)),
- _ => jenkins_params.push((¶m_name, ¶m_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)
}
}
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com IBM Australia Limited
More information about the snowpatch
mailing list