Refactor with the help of Clippy (#462)

We add clippy as our build — also rectifying the missing `plume-cli` build!

In the next step we follow clippy's advise and fix some of the "simple" mistakes in our code, such as style or map usage.

Finally, we refactor some hard bits that need extraction of new types, or refactoring of function call-types, especially those that thread thru macros, and, of course functions with ~15 parameters should probably be rethought.

.travis.yml

@@ -27,17 +27,36 @@ jobs:
   include:
     - stage: build
       name: "Build front"
-      script: (cargo web -h || cargo install cargo-web) && cd plume-front && cargo web check
+      script: (cargo web -h || cargo install cargo-web) && cd plume-front && cargo clippy -- -D warnings && cargo web check
+      before_script: rustup component add clippy
+
     - stage: build
       name: "Build with postgresql"
       env:
         - MIGRATION_DIR=migrations/postgres FEATURES=postgres DATABASE_URL=postgres://postgres@localhost/plume
-      script: cargo build --no-default-features --features="${FEATURES}" --release
+      script: cargo clippy --no-default-features --features="${FEATURES}" --release -- -D warnings
+      before_script: rustup component add clippy
+
+    - stage: build
+      name: "Build CLI with postgresql"
+      env:
+        - MIGRATION_DIR=migrations/postgres FEATURES=postgres DATABASE_URL=postgres://postgres@localhost/plume
+      script: cd plume-cli && cargo clippy --no-default-features --features="${FEATURES}" --release -- -D warnings
+      before_script: rustup component add clippy
     - stage: build
       name: "Build with sqlite"
       env:
         - MIGRATION_DIR=migrations/sqlite   FEATURES=sqlite   DATABASE_URL=plume.sqlite3
-      script: cargo build --no-default-features --features="${FEATURES}" --release
+      script: cargo clippy --no-default-features --features="${FEATURES}" --release -- -D warnings
+      before_script: rustup component add clippy
+
+    - stage: build
+      name: "Build CLI with sqlite"
+      env:
+        - MIGRATION_DIR=migrations/sqlite   FEATURES=sqlite   DATABASE_URL=plume.sqlite3
+      script: cd plume-cli && cargo clippy --no-default-features --features="${FEATURES}" --release -- -D warnings
+      before_script: rustup component add clippy
+
     - stage: test and coverage
       name: "Test with potgresql backend"
       env:
@@ -48,6 +67,7 @@ jobs:
         - |
             cargo test --features "${FEATURES}" --no-default-features  --all --exclude plume-front &&
             ./script/compute_coverage.sh
+
     - stage: test and coverage
       name: "Test with Sqlite backend"
       env:

plume-cli/src/search.rs

@@ -60,13 +60,7 @@ fn init<'a>(args: &ArgMatches<'a>, conn: &Connection) {
     let path = Path::new(path).join("search_index");
 
     let can_do = match read_dir(path.clone()) { // try to read the directory specified
-        Ok(mut contents) => {
-            if contents.next().is_none()  {
-                true
-            } else {
-                false
-            }
-        },
+        Ok(mut contents) =>  contents.next().is_none(),
         Err(e) => if e.kind() == ErrorKind::NotFound {
             true
         } else {
@@ -107,5 +101,3 @@ fn unlock<'a>(args: &ArgMatches<'a>) {
 
     remove_file(path).unwrap();
 }
-
-

plume-front/src/editor.rs

@@ -72,7 +72,7 @@ fn init_widget(
     }
     widget.append_child(&document().create_text_node(&content));
     if disable_return {
-        widget.add_event_listener(no_return);        
+        widget.add_event_listener(no_return);
     }
 
     parent.append_child(&widget);
@@ -128,7 +128,7 @@ pub fn init() -> Result<(), EditorError> {
 
             popup.class_list().add("show").unwrap();
             bg.class_list().add("show").unwrap();
-        }));        
+        }));
     }
     Ok(())
 }
@@ -233,7 +233,7 @@ fn make_editable(tag: &'static str) -> Element {
     elt
 }
 
-fn placeholder<'a>(elt: HtmlElement, text: &'a str) -> HtmlElement {
+fn placeholder(elt: HtmlElement, text: &str) -> HtmlElement {
     elt.dataset().insert("placeholder", text).unwrap();
     elt.dataset().insert("edited", "false").unwrap();
 
@@ -248,7 +248,7 @@ fn placeholder<'a>(elt: HtmlElement, text: &'a str) -> HtmlElement {
 
             let ph = document().create_element("span").expect("Couldn't create placeholder");
             ph.class_list().add("placeholder").expect("Couldn't add class");
-            ph.append_child(&document().create_text_node(&elt.dataset().get("placeholder").unwrap_or(String::new())));
+            ph.append_child(&document().create_text_node(&elt.dataset().get("placeholder").unwrap_or_default()));
             elt.append_child(&ph);
         }
     }));

plume-front/src/main.rs

@@ -38,37 +38,34 @@ fn main() {
 /// It should normally be working fine even without this code
 /// But :focus-within is not yet supported by Webkit/Blink
 fn menu() {
-    document().get_element_by_id("menu")
-        .map(|button| {
-            document().get_element_by_id("content")
-                .map(|menu| {
-                    button.add_event_listener(|_: ClickEvent| {
-                        document().get_element_by_id("menu").map(|menu| menu.class_list().add("show"));
-                    });
-                    menu.add_event_listener(|_: ClickEvent| {
-                        document().get_element_by_id("menu").map(|menu| menu.class_list().remove("show"));
-                    });
-                })
-        });
+    if let Some(button) = document().get_element_by_id("menu") {
+        if let Some(menu) = document().get_element_by_id("content") {
+            button.add_event_listener(|_: ClickEvent| {
+                document().get_element_by_id("menu").map(|menu| menu.class_list().add("show"));
+            });
+            menu.add_event_listener(|_: ClickEvent| {
+                document().get_element_by_id("menu").map(|menu| menu.class_list().remove("show"));
+            });
+        }
+    }
 }
 
 /// Clear the URL of the search page before submitting request
 fn search() {
-    document().get_element_by_id("form")
-        .map(|form| {
-            form.add_event_listener(|_: SubmitEvent| {
-                document().query_selector_all("#form input").map(|inputs| {
-                    for input in inputs {
-                        js! {
-                            if (@{&input}.name === "") {
-                                @{&input}.name = @{&input}.id
-                            }
-                            if (@{&input}.name && !@{&input}.value) {
-                                @{&input}.name = "";
-                            }
+    if let Some(form) = document().get_element_by_id("form") {
+        form.add_event_listener(|_: SubmitEvent| {
+            document().query_selector_all("#form input").map(|inputs| {
+                for input in inputs {
+                    js! {
+                        if (@{&input}.name === "") {
+                            @{&input}.name = @{&input}.id
+                        }
+                        if (@{&input}.name && !@{&input}.value) {
+                            @{&input}.name = "";
                         }
                     }
-                }).ok();
-            });
+                }
+            }).ok();
         });
+    }
 }

plume-models/src/api_tokens.rs

@@ -90,9 +90,9 @@ impl<'a, 'r> FromRequest<'a, 'r> for ApiToken {
 
         let mut parsed_header = headers[0].split(' ');
         let auth_type = parsed_header.next()
-            .map_or_else(|| Outcome::Failure((Status::BadRequest, TokenError::NoType)), |t| Outcome::Success(t))?;
+            .map_or_else(|| Outcome::Failure((Status::BadRequest, TokenError::NoType)), Outcome::Success)?;
         let val = parsed_header.next()
-            .map_or_else(|| Outcome::Failure((Status::BadRequest, TokenError::NoValue)), |t| Outcome::Success(t))?;
+            .map_or_else(|| Outcome::Failure((Status::BadRequest, TokenError::NoValue)), Outcome::Success)?;
 
         if auth_type == "Bearer" {
             let conn = request.guard::<DbConn>().map_failure(|_| (Status::InternalServerError, TokenError::DbError))?;

plume-models/src/lib.rs

@@ -1,4 +1,5 @@
 #![feature(try_trait)]
+#![feature(never_type)]
 
 extern crate activitypub;
 extern crate ammonia;

plume-models/src/medias.rs

@@ -70,8 +70,8 @@ impl Media {
     pub fn page_for_user(conn: &Connection, user: &User, (min, max): (i32, i32)) -> Result<Vec<Media>> {
         medias::table
             .filter(medias::owner_id.eq(user.id))
-            .offset(min as i64)
-            .limit((max - min) as i64)
+            .offset(i64::from(min))
+            .limit(i64::from(max - min))
             .load::<Media>(conn)
             .map_err(Error::from)
     }

plume-models/src/posts.rs

@@ -127,11 +127,11 @@ impl<'a> Provider<(&'a Connection, &'a Worker, &'a Searcher, Option<i32>)> for P
                 published: Some(p.published),
                 creation_date: Some(p.creation_date.format("%Y-%m-%d").to_string()),
                 license: Some(p.license.clone()),
-                tags: Some(Tag::for_post(conn, p.id).unwrap_or(vec![]).into_iter().map(|t| t.tag).collect()),
+                tags: Some(Tag::for_post(conn, p.id).unwrap_or_else(|_| vec![]).into_iter().map(|t| t.tag).collect()),
                 cover_id: p.cover_id,
             })
             .collect()
-        ).unwrap_or(vec![])
+        ).unwrap_or_else(|_| vec![])
     }
 
     fn update(
@@ -146,7 +146,7 @@ impl<'a> Provider<(&'a Connection, &'a Worker, &'a Searcher, Option<i32>)> for P
         let user_id = user_id.expect("Post as Provider::delete: not authenticated");
         if let Ok(post) = Post::get(conn, id) {
             if post.is_author(conn, user_id).unwrap_or(false) {
-                post.delete(&(conn, search)).ok().expect("Post as Provider::delete: delete error");
+                post.delete(&(conn, search)).expect("Post as Provider::delete: delete error");
             }
         }
     }
@@ -168,7 +168,7 @@ impl<'a> Provider<(&'a Connection, &'a Worker, &'a Searcher, Option<i32>)> for P
         let domain = &Instance::get_local(&conn)
             .map_err(|_| ApiError::NotFound("posts::update: Error getting local instance".into()))?
             .public_domain;
-        let (content, mentions, hashtags) = md_to_html(query.source.clone().unwrap_or(String::new()).clone().as_ref(), domain);
+        let (content, mentions, hashtags) = md_to_html(query.source.clone().unwrap_or_default().clone().as_ref(), domain);
 
         let author = User::get(conn, user_id.expect("<Post as Provider>::create: no user_id error"))
             .map_err(|_| ApiError::NotFound("Author not found".into()))?;
@@ -185,16 +185,16 @@ impl<'a> Provider<(&'a Connection, &'a Worker, &'a Searcher, Option<i32>)> for P
 
         let post = Post::insert(conn, NewPost {
             blog_id: blog,
-            slug: slug,
-            title: title,
+            slug,
+            title,
             content: SafeString::new(content.as_ref()),
             published: query.published.unwrap_or(true),
-            license: query.license.unwrap_or(Instance::get_local(conn)
+            license: query.license.unwrap_or_else(|| Instance::get_local(conn)
                 .map(|i| i.default_license)
-                .unwrap_or(String::from("CC-BY-SA"))),
+                .unwrap_or_else(|_| String::from("CC-BY-SA"))),
             creation_date: date,
             ap_url: String::new(),
-            subtitle: query.subtitle.unwrap_or(String::new()),
+            subtitle: query.subtitle.unwrap_or_default(),
             source: query.source.expect("Post API::create: no source error"),
             cover_id: query.cover_id,
         }, search).map_err(|_| ApiError::NotFound("Creation error".into()))?;
@@ -207,7 +207,7 @@ impl<'a> Provider<(&'a Connection, &'a Worker, &'a Searcher, Option<i32>)> for P
         if let Some(tags) = query.tags {
             for tag in tags {
                 Tag::insert(conn, NewTag {
-                    tag: tag,
+                    tag,
                     is_hashtag: false,
                     post_id: post.id
                 }).map_err(|_| ApiError::NotFound("Error saving tags".into()))?;
@@ -311,7 +311,7 @@ impl Post {
             .load(conn)?
             .iter()
             .next()
-            .map(|x| *x)
+            .cloned()
             .ok_or(Error::NotFound)
     }
 
@@ -908,7 +908,7 @@ impl<'a> FromActivity<LicensedArticle, (&'a Connection, &'a Searcher)> for Post
                             .content_string()?,
                     ),
                     published: true,
-                    license: license,
+                    license,
                     // FIXME: This is wrong: with this logic, we may use the display URL as the AP ID. We need two different fields
                     ap_url: article.object_props.url_string().or_else(|_|
                         article

plume-models/src/safe_string.rs

@@ -24,7 +24,7 @@ lazy_static! {
             .url_relative(UrlRelative::Custom(Box::new(url_add_prefix)))
             .add_tag_attributes(
                 "iframe",
-                [ "width", "height", "src", "frameborder" ].iter().map(|&v| v),
+                [ "width", "height", "src", "frameborder" ].iter().cloned(),
             )
             .add_tag_attributes(
                 "video",

plume-models/src/search/mod.rs

@@ -9,6 +9,7 @@ pub use self::query::PlumeQuery as Query;
 pub(crate) mod tests {
     use super::{Query, Searcher};
     use std::env::temp_dir;
+    use std::str::FromStr;
     use diesel::Connection;
 
     use plume_common::activity_pub::inbox::Deletable;
@@ -65,7 +66,7 @@ pub(crate) mod tests {
             ("after:2017-11-05 after:2018-01-01", "after:2018-01-01"),
         ];
         for (source, res) in vector {
-            assert_eq!(&Query::from_str(source).to_string(), res);
+            assert_eq!(&Query::from_str(source).unwrap().to_string(), res);
             assert_eq!(Query::new().parse_query(source).to_string(), res);
         }
     }
@@ -141,18 +142,18 @@ pub(crate) mod tests {
             }).unwrap();
 
             searcher.commit();
-            assert_eq!(searcher.search_document(conn, Query::from_str(&title), (0,1))[0].id, post.id);
+            assert_eq!(searcher.search_document(conn, Query::from_str(&title).unwrap(), (0,1))[0].id, post.id);
 
             let newtitle = random_hex()[..8].to_owned();
             post.title = newtitle.clone();
             post.update(conn, &searcher).unwrap();
             searcher.commit();
-            assert_eq!(searcher.search_document(conn, Query::from_str(&newtitle), (0,1))[0].id, post.id);
-            assert!(searcher.search_document(conn, Query::from_str(&title), (0,1)).is_empty());
+            assert_eq!(searcher.search_document(conn, Query::from_str(&newtitle).unwrap(), (0,1))[0].id, post.id);
+            assert!(searcher.search_document(conn, Query::from_str(&title).unwrap(), (0,1)).is_empty());
 
             post.delete(&(conn, &searcher)).unwrap();
             searcher.commit();
-            assert!(searcher.search_document(conn, Query::from_str(&newtitle), (0,1)).is_empty());
+            assert!(searcher.search_document(conn, Query::from_str(&newtitle).unwrap(), (0,1)).is_empty());
 
             Ok(())
         });