Little Refactors (#683)

* Refactors!
* Removed duplicate CRATESFYI_PREFIX variable

Co-Authored-By: Joshua Nelson <[email protected]>

* Split reponame into a separate variable
* Return query error
* Changed ok_or_else to ok_or
* Annotated &&str dereferences with reasoning

Author: Kixiron

build.rs

@@ -19,6 +19,7 @@ fn main() {
 fn write_git_version() {
     let maybe_hash = get_git_hash();
     let git_hash = maybe_hash.as_deref().unwrap_or("???????");
+
     let build_date = time::strftime("%Y-%m-%d", &time::now_utc()).unwrap();
     let dest_path = Path::new(&env::var("OUT_DIR").unwrap()).join("git_version");
 

src/db/add_package.rs

@@ -410,6 +410,7 @@ fn add_keywords_into_database(
                 .get(0)
             }
         };
+
         // add releationship
         let _ = conn.query(
             "INSERT INTO keyword_rels (rid, kid) VALUES ($1, $2)",

src/db/file.rs

@@ -127,13 +127,15 @@ pub(super) fn s3_client() -> Option<S3Client> {
     if std::env::var_os("AWS_ACCESS_KEY_ID").is_none() && std::env::var_os("FORCE_S3").is_none() {
         return None;
     }
+
     let creds = match DefaultCredentialsProvider::new() {
         Ok(creds) => creds,
         Err(err) => {
             warn!("failed to retrieve AWS credentials: {}", err);
             return None;
         }
     };
+
     Some(S3Client::new_with(
         rusoto_core::request::HttpClient::new().unwrap(),
         creds,

src/docbuilder/crates.rs

@@ -18,13 +18,16 @@ where
     for line in reader.lines() {
         // some crates have invalid UTF-8 (nanny-sys-0.0.7)
         // skip them
-        let line = match line {
-            Ok(l) => l,
-            Err(_) => continue,
+        let line = if let Ok(line) = line {
+            line
+        } else {
+            continue;
         };
-        let data = match Json::from_str(line.trim()) {
-            Ok(d) => d,
-            Err(_) => continue,
+
+        let data = if let Ok(data) = Json::from_str(line.trim()) {
+            data
+        } else {
+            continue;
         };
 
         let obj = data

src/docbuilder/mod.rs

@@ -38,6 +38,7 @@ impl DocBuilder {
     /// Loads build cache
     pub fn load_cache(&mut self) -> Result<()> {
         debug!("Loading cache");
+
         let path = PathBuf::from(&self.options.prefix).join("cache");
         let reader = fs::File::open(path).map(BufReader::new);
 
@@ -54,6 +55,7 @@ impl DocBuilder {
 
     fn load_database_cache(&mut self) -> Result<()> {
         debug!("Loading database cache");
+
         use crate::db::connect_db;
         let conn = connect_db()?;
 
@@ -64,6 +66,7 @@ impl DocBuilder {
         )? {
             let name: String = row.get(0);
             let version: String = row.get(1);
+
             self.db_cache.insert(format!("{}-{}", name, version));
         }
 
@@ -73,11 +76,14 @@ impl DocBuilder {
     /// Saves build cache
     pub fn save_cache(&self) -> Result<()> {
         debug!("Saving cache");
+
         let path = PathBuf::from(&self.options.prefix).join("cache");
         let mut file = fs::OpenOptions::new().write(true).create(true).open(path)?;
+
         for krate in &self.cache {
             writeln!(file, "{}", krate)?;
         }
+
         Ok(())
     }
 
@@ -91,6 +97,7 @@ impl DocBuilder {
         if !path.exists() {
             fs::OpenOptions::new().write(true).create(true).open(path)?;
         }
+
         Ok(())
     }
 
@@ -100,6 +107,7 @@ impl DocBuilder {
         if path.exists() {
             fs::remove_file(path)?;
         }
+
         Ok(())
     }
 
@@ -121,6 +129,7 @@ impl DocBuilder {
         let name = format!("{}-{}", name, version);
         let local = self.options.skip_if_log_exists && self.cache.contains(&name);
         let db = self.options.skip_if_exists && self.db_cache.contains(&name);
+
         !(local || db)
     }
 }

src/docbuilder/options.rs

@@ -55,6 +55,7 @@ impl DocBuilderOptions {
     /// Creates new DocBuilderOptions from prefix
     pub fn from_prefix(prefix: PathBuf) -> DocBuilderOptions {
         let (prefix, crates_io_index_path) = generate_paths(prefix);
+
         DocBuilderOptions {
             prefix,
             crates_io_index_path,
@@ -69,6 +70,7 @@ impl DocBuilderOptions {
                 self.crates_io_index_path.display()
             );
         }
+
         Ok(())
     }
 }

src/docbuilder/rustwide_builder.rs

@@ -122,8 +122,9 @@ impl RustwideBuilder {
 
         let mut targets_to_install = TARGETS
             .iter()
-            .map(|&t| t.to_string())
+            .map(|&t| t.to_string()) // &str has a specialized ToString impl, while &&str goes through Display
             .collect::<HashSet<_>>();
+
         let installed_targets = match self.toolchain.installed_targets(&self.workspace) {
             Ok(targets) => targets,
             Err(err) => {

src/utils/daemon.rs

@@ -19,16 +19,16 @@ use time;
 use ::{libc::fork, std::fs::File, std::io::Write, std::process::exit};
 
 pub fn start_daemon(background: bool) {
-    // first check required environment variables
-    for v in [
+    const CRATE_VARIABLES: [&str; 3] = [
         "CRATESFYI_PREFIX",
         "CRATESFYI_GITHUB_USERNAME",
         "CRATESFYI_GITHUB_ACCESSTOKEN",
-    ]
-    .iter()
-    {
+    ];
+
+    // first check required environment variables
+    for v in CRATE_VARIABLES.iter() {
         if env::var(v).is_err() {
-            panic!("Environment variable {} not found", v);
+            panic!("Environment variable {} not found", v)
         }
     }
 
@@ -42,6 +42,7 @@ pub fn start_daemon(background: bool) {
         {
             panic!("running in background not supported on windows");
         }
+
         #[cfg(not(target_os = "windows"))]
         {
             // fork the process
@@ -87,6 +88,7 @@ pub fn start_daemon(background: bool) {
         .unwrap();
 
     // build new crates every minute
+    // REFACTOR: Break this into smaller functions
     thread::Builder::new().name("build queue reader".to_string()).spawn(move || {
         let opts = opts();
         let mut doc_builder = DocBuilder::new(opts);
@@ -146,6 +148,7 @@ pub fn start_daemon(background: bool) {
                     error!("Failed to read the number of crates in the queue: {}", e);
                     continue;
                 }
+
                 Ok(0) => {
                     if status.count() > 0 {
                         // ping the hubs before continuing
@@ -162,6 +165,7 @@ pub fn start_daemon(background: bool) {
                     status = BuilderState::EmptyQueue;
                     continue;
                 }
+
                 Ok(queue_count) => {
                     info!("Starting build with {} crates in queue (currently on a {} crate streak)",
                           queue_count, status.count());
@@ -185,7 +189,6 @@ pub fn start_daemon(background: bool) {
                     Ok(crate_built) => if crate_built {
                         status.increment();
                     }
-
                 }
             }));
 
@@ -257,6 +260,7 @@ pub fn start_daemon(background: bool) {
 
     // at least start web server
     info!("Starting web server");
+
     crate::Server::start(None);
 }
 

src/utils/github_updater.rs

@@ -136,16 +136,16 @@ fn get_github_path(url: &str) -> Option<String> {
         Some(cap) => {
             let username = cap.get(1).unwrap().as_str();
             let reponame = cap.get(2).unwrap().as_str();
-            Some(format!(
-                "{}/{}",
-                username,
-                if reponame.ends_with(".git") {
-                    reponame.split(".git").next().unwrap()
-                } else {
-                    reponame
-                }
-            ))
+
+            let reponame = if reponame.ends_with(".git") {
+                reponame.split(".git").next().unwrap()
+            } else {
+                reponame
+            };
+
+            Some(format!("{}/{}", username, reponame))
         }
+
         None => None,
     }
 }

src/utils/html.rs

@@ -32,13 +32,15 @@ fn extract_from_rcdom(dom: &RcDom) -> Result<(Handle, Handle)> {
                         head = Some(handle.clone());
                     }
                 }
+
                 "body" => {
                     if body.is_some() {
                         return Err(err_msg("duplicate <body> tag"));
                     } else {
                         body = Some(handle.clone());
                     }
                 }
+
                 _ => {} // do nothing
             }
         }
@@ -68,6 +70,7 @@ fn extract_class(node: &Handle) -> String {
                 .find(|a| &a.name.local == "class")
                 .map_or(String::new(), |a| a.value.to_string())
         }
+
         _ => String::new(),
     }
 }
@@ -79,6 +82,7 @@ mod test {
         let (head, body, class) = super::extract_head_and_body(
             r#"<head><meta name="generator" content="rustdoc"></head><body class="rustdoc struct"><p>hello</p>"#
         ).unwrap();
+
         assert_eq!(head, r#"<meta name="generator" content="rustdoc">"#);
         assert_eq!(body, "<p>hello</p>");
         assert_eq!(class, "rustdoc struct");
@@ -91,6 +95,7 @@ mod test {
         let expected_head = std::fs::read_to_string("tests/regex/head.html").unwrap();
         let expected_body = std::fs::read_to_string("tests/regex/body.html").unwrap();
         let (head, body, class) = super::extract_head_and_body(&original).unwrap();
+
         assert_eq!(head, expected_head.trim());
         assert_eq!(&body, &expected_body.trim());
         assert_eq!(class, "rustdoc struct");

src/utils/release_activity_updater.rs

@@ -6,17 +6,17 @@ use time::{now, Duration};
 
 pub fn update_release_activity() -> Result<()> {
     let conn = connect_db()?;
-    let mut dates = Vec::new();
-    let mut crate_counts = Vec::new();
-    let mut failure_counts = Vec::new();
+    let mut dates = Vec::with_capacity(30);
+    let mut crate_counts = Vec::with_capacity(30);
+    let mut failure_counts = Vec::with_capacity(30);
 
     for day in 0..30 {
         let rows = conn.query(
             &format!(
                 "SELECT COUNT(*)
-                                             FROM releases
-                                             WHERE release_time < NOW() - INTERVAL '{} day' AND
-                                                   release_time > NOW() - INTERVAL '{} day'",
+                 FROM releases
+                 WHERE release_time < NOW() - INTERVAL '{} day' AND
+                       release_time > NOW() - INTERVAL '{} day'",
                 day,
                 day + 1
             ),
@@ -25,22 +25,24 @@ pub fn update_release_activity() -> Result<()> {
         let failures_count_rows = conn.query(
             &format!(
                 "SELECT COUNT(*)
-                                             FROM releases
-                                             WHERE is_library = TRUE AND
-                                                   build_status = FALSE AND
-                                                   release_time < NOW() - INTERVAL '{} day' AND
-                                                   release_time > NOW() - INTERVAL '{} day'",
+                 FROM releases
+                 WHERE is_library = TRUE AND
+                       build_status = FALSE AND
+                       release_time < NOW() - INTERVAL '{} day' AND
+                       release_time > NOW() - INTERVAL '{} day'",
                 day,
                 day + 1
             ),
             &[],
         )?;
+
         let release_count: i64 = rows.get(0).get(0);
         let failure_count: i64 = failures_count_rows.get(0).get(0);
         let now = now();
         let date = now - Duration::days(day);
+
+        // unwrap is fine here, as our date format is always valid
         dates.push(format!("{}", date.strftime("%d %b").unwrap()));
-        // unwrap is fine here,             ~~~~~~~~~~~~^  our date format is always valid
         crate_counts.push(release_count);
         failure_counts.push(failure_count);
     }
@@ -54,6 +56,7 @@ pub fn update_release_activity() -> Result<()> {
         map.insert("dates".to_owned(), dates.to_json());
         map.insert("counts".to_owned(), crate_counts.to_json());
         map.insert("failures".to_owned(), failure_counts.to_json());
+
         map.to_json()
     };
 

src/web/crate_details.rs

@@ -163,10 +163,10 @@ impl CrateDetails {
                     if let Some(version) = version.as_string() {
                         if let Ok(sem_ver) = semver::Version::parse(&version) {
                             versions.push(sem_ver);
-                        };
-                    };
+                        }
+                    }
                 }
-            };
+            }
 
             versions.sort();
             versions.reverse();

src/web/page.rs

@@ -114,6 +114,7 @@ impl<T: ToJson> Page<T> {
         let status = self.status;
         let temp = Template::new(template, self);
         resp.set_mut(temp).set_mut(status);
+
         Ok(resp)
     }
 }

src/web/releases.rs

@@ -447,6 +447,7 @@ pub fn author_handler(req: &mut Request) -> IronResult<Response> {
 
     let (author_name, packages) = if author.starts_with('@') {
         let mut author = author.split('@');
+
         get_releases_by_owner(
             &conn,
             page_number,
@@ -572,7 +573,7 @@ pub fn search_handler(req: &mut Request) -> IronResult<Response> {
         let search_query = query.replace(" ", " & ");
         #[allow(clippy::or_fun_call)]
         get_search_results(&conn, &search_query, 1, RELEASES_IN_RELEASES)
-            .ok_or(IronError::new(Nope::NoResults, status::NotFound))
+            .ok_or_else(|| IronError::new(Nope::NoResults, status::NotFound))
             .and_then(|(_, results)| {
                 // FIXME: There is no pagination
                 Page::new(results)

src/web/routes.rs

@@ -6,6 +6,7 @@ use std::collections::HashSet;
 
 const DOC_RUST_LANG_ORG_REDIRECTS: &[&str] = &["alloc", "core", "proc_macro", "std", "test"];
 
+// REFACTOR: Break this into smaller initialization functions
 pub(super) fn build_routes() -> Routes {
     let mut routes = Routes::new();