Refactor with the help of Clippy #462

trinity-1686a · 2019-03-08T22:03:31Z

igalic commented

2019-03-08 22:03:31 +00:00

(Migrated from github.com)

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.

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.

👍 2 ❤️ 1 🎉 1

trinity-1686a commented

2019-03-08 23:42:39 +00:00

(Most functions with ~15 parameters are routes, we won't be able to fix these warnings)

codecov[bot] commented

2019-03-09 00:09:20 +00:00

(Migrated from github.com)

Codecov Report

Merging #462 into master will increase coverage by 0.27%.
The diff coverage is 17.81%.

@@            Coverage Diff            @@
##           master    #462      +/-   ##
=========================================
+ Coverage   27.02%   27.3%   +0.27%     
=========================================
  Files          64      64              
  Lines        7431    7582     +151     
=========================================
+ Hits         2008    2070      +62     
- Misses       5423    5512      +89

# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/462?src=pr&el=h1) Report > Merging [#462](https://codecov.io/gh/Plume-org/Plume/pull/462?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/570d7fe2d06eaf50c25ff15bac835e0d26cc78a6?src=pr&el=desc) will **increase** coverage by `0.27%`. > The diff coverage is `17.81%`. ```diff @@ Coverage Diff @@ ## master #462 +/- ## ========================================= + Coverage 27.02% 27.3% +0.27% ========================================= Files 64 64 Lines 7431 7582 +151 ========================================= + Hits 2008 2070 +62 - Misses 5423 5512 +89 ```

igalic commented

2019-03-09 07:30:45 +00:00

(Migrated from github.com)

couldn't we extract some of those parameters into a Struct?

trinity-1686a commented

2019-03-09 08:08:52 +00:00

We could create a struct containing DbConn, Worker, I18n, Searcher and Option<User>(current connected user). Most routes requires at least 3 of these, so requires all

We could create a struct containing `DbConn`, `Worker`, `I18n`, `Searcher` and `Option<User>`(current connected user). Most routes requires at least 3 of these, so requires all

👍 2

igalic commented

2019-03-10 21:45:25 +00:00

(Migrated from github.com)

We could create a struct containing DbConn, Worker, I18n, Searcher and Option<User>(current connected user). Most routes requires at least 3 of these, so requires all

I'm thinking of this:

pub struct PlumeRocket {
    slug: String,
    conn: DbConn,
    user: Option<User>,
    intl: I18n,
    worker: Option<Worker>,
    searcher: Option<Searcher>,
}

i'm not sure about the name of the struct yet (or where to put it)

> We could create a struct containing `DbConn`, `Worker`, `I18n`, `Searcher` and `Option<User>`(current connected user). Most routes requires at least 3 of these, so requires all I'm thinking of this: ```rust pub struct PlumeRocket { slug: String, conn: DbConn, user: Option<User>, intl: I18n, worker: Option<Worker>, searcher: Option<Searcher>, } ``` i'm not sure about the name of the `struct` yet (or where to put it)

elegaanz (Migrated from github.com) reviewed 2019-03-13 14:45:45 +00:00

elegaanz (Migrated from github.com) left a comment

Really looks better! I'm not 100% sure about the PlumeRocket name, I would have named it AppState or something like that, but I don't know maybe it's not really important. Otherwise it is really great as I said. 👍

Really looks better! I'm not 100% sure about the `PlumeRocket` name, I would have named it `AppState` or something like that, but I don't know maybe it's not really important. Otherwise it is really great as I said. :+1:

src/routes/blogs.rs

					
				@ -33,3 +33,4 @@

				    let intl = rockets.intl;

				    Ok(render!(blogs::details(

				        &(&*conn, &intl.catalog, user.clone()),

elegaanz (Migrated from github.com) commented

2019-03-13 14:40:40 +00:00

Maybe Rust supports destructuring (I don't remember to be honest, so you could try to do:

let { user, intl } = rockets;

Maybe Rust supports destructuring (I don't remember to be honest, so you could try to do: ```rust let { user, intl } = rockets; ```

src/routes/blogs.rs

					
				@ -123,3 +131,3 @@

				#[post("/~/<name>/delete")]

				pub fn delete(conn: DbConn, name: String, user: Option<User>, intl: I18n, searcher: Searcher) -> Result<Redirect, Ructe>{

				pub fn delete(name: String, rockets: PlumeRocket) -> Result<Redirect, Ructe>{

elegaanz (Migrated from github.com) commented

2019-03-13 14:43:06 +00:00

Maybe in another PR (because I think it is a bit out of scope for this one), we could replace all the "login required" routes with something like:

rockets.user.ok_or(Err(ErrorPage::Unauthorized("You need to be logged in to do this")))?;

(I don't remember if ok_or is the correct function to use here, but I think you got what I wanted to do)

Maybe in another PR (because I think it is a bit out of scope for this one), we could replace all the "login required" routes with something like: ```rust rockets.user.ok_or(Err(ErrorPage::Unauthorized("You need to be logged in to do this")))?; ``` (I don't remember if `ok_or` is the correct function to use here, but I think you got what I wanted to do)

trinity-1686a reviewed 2019-03-13 15:57:56 +00:00

src/routes/mod.rs

					
				@ -1,3 +1,4 @@

				#![warn(clippy::too_many_arguments)]

				use atom_syndication::{ContentBuilder, Entry, EntryBuilder, LinkBuilder, Person, PersonBuilder};

				use rocket::{

				    http::{

trinity-1686a commented

2019-03-13 15:57:56 +00:00

have you tried asking for a PlumeRocket while not logged in? I think this would fail because of this line

trinity-1686a reviewed 2019-03-15 15:57:18 +00:00

trinity-1686a left a comment

There is a lot of work in there.
The only truly blocking thing is PlumeRocket's guard failing when user is not logged in, I through you had fixed that but apparently you didn't yet

There is a lot of work in there. The only truly blocking thing is PlumeRocket's guard failing when user is not logged in, I through you had fixed that but apparently you didn't yet

plume-models/src/search/query.rs

					
				@ -323,4 +327,4 @@

				impl ToString for PlumeQuery {

				    fn to_string(&self) -> String {

				        let mut result = String::new();

trinity-1686a commented

2019-03-15 15:28:49 +00:00

seeing parse_query, I think this function could be moved into it, and then we can call parse_query instead of from_str_req

seeing `parse_query`, I think this function could be moved into it, and then we can call `parse_query` instead of `from_str_req`

src/routes/blogs.rs

					
				@ -33,3 +33,4 @@

				    let intl = rockets.intl;

				    Ok(render!(blogs::details(

				        &(&*conn, &intl.catalog, user.clone()),

trinity-1686a commented

2019-03-15 15:42:54 +00:00

Your syntax is not right, but there is a way https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=572b846e985d4780607766e40563e6ba
(This is supposed to be a reply to https://github.com/Plume-org/Plume/pull/462#discussion_r265160941 but I failed apparently)

Your syntax is not right, but there is a way https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=572b846e985d4780607766e40563e6ba (This is supposed to be a reply to https://github.com/Plume-org/Plume/pull/462#discussion_r265160941 but I failed apparently)

src/routes/mod.rs

trinity-1686a commented

2019-03-15 15:48:24 +00:00

These could be Searcher<'a> and Worker<'a> instead of Option<...>

These could be `Searcher<'a>` and `Worker<'a>` instead of `Option<...>`

src/routes/mod.rs

					
				@ -1,3 +1,4 @@

				#![warn(clippy::too_many_arguments)]

				use atom_syndication::{ContentBuilder, Entry, EntryBuilder, LinkBuilder, Person, PersonBuilder};

				use rocket::{

				    http::{