aitzol/Plume
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Refactor with the help of Clippy (#462)

Browse Source 
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.
This commit is contained in:
Igor Galić
2019-03-19 14:37:56 +01:00
committed by Baptiste Gelez
parent 570d7fe2d0
commit 732f514da7
34 changed files with 393 additions and 297 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plume-models/src/api_tokens.rs
+2 -2
View File
@@ -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
View File
@@ -1,4 +1,5 @@
#![feature(try_trait)]
#![feature(never_type)]
extern crate activitypub;
extern crate ammonia;
plume-models/src/medias.rs
+2 -2
View File
@@ -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
+12 -12
View File
@@ -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
+1 -1
View File
@@ -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
+6 -5
View File
@@ -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(())
});
plume-models/src/search/query.rs
+57 -54
View File
@@ -87,9 +87,9 @@ macro_rules! gen_to_string {
$(
for (occur, val) in &$self.$field {
if val.contains(' ') {
$result.push_str(&format!("{}{}:\"{}\" ", Self::occur_to_str(&occur), stringify!($field), val));
$result.push_str(&format!("{}{}:\"{}\" ", Self::occur_to_str(*occur), stringify!($field), val));
} else {
$result.push_str(&format!("{}{}:{} ", Self::occur_to_str(&occur), stringify!($field), val));
$result.push_str(&format!("{}{}:{} ", Self::occur_to_str(*occur), stringify!($field), val));
}
}
)*
@@ -148,20 +148,6 @@ impl PlumeQuery {
Default::default()
}
/// Create a new Query from &str
/// Same as doing
/// ```rust
/// # extern crate plume_models;
/// # use plume_models::search::Query;
/// let mut q = Query::new();
/// q.parse_query("some query");
/// ```
pub fn from_str(query: &str) -> Self {
let mut res: Self = Default::default();
res.from_str_req(&query.trim());
res
}
/// Parse a query string into this Query
pub fn parse_query(&mut self, query: &str) -> &mut Self {
@@ -222,35 +208,31 @@ impl PlumeQuery {
}
// split a string into a token and a rest
pub fn get_first_token<'a>(mut query: &'a str) -> (&'a str, &'a str) {
pub fn get_first_token(mut query: &str) -> (&str, &str) {
query = query.trim();
if query.is_empty() {
("", "")
} else {
if query.get(0..1).map(|v| v=="\"").unwrap_or(false) {
if let Some(index) = query[1..].find('"') {
query.split_at(index+2)
} else {
(query, "")
}
} else if query.get(0..2).map(|v| v=="+\"" || v=="-\"").unwrap_or(false) {
if let Some(index) = query[2..].find('"') {
query.split_at(index+3)
} else {
(query, "")
}
} else {
if let Some(index) = query.find(' ') {
query.split_at(index)
} else if query.get(0..1).map(|v| v=="\"").unwrap_or(false) {
if let Some(index) = query[1..].find('"') {
query.split_at(index+2)
} else {
(query, "")
}
}
} else if query.get(0..2).map(|v| v=="+\"" || v=="-\"").unwrap_or(false) {
if let Some(index) = query[2..].find('"') {
query.split_at(index+3)
} else {
(query, "")
}
} else if let Some(index) = query.find(' ') {
query.split_at(index)
} else {
(query, "")
}
}
// map each Occur state to a prefix
fn occur_to_str(occur: &Occur) -> &'static str {
fn occur_to_str(occur: Occur) -> &'static str {
match occur {
Occur::Should => "",
Occur::Must => "+",
@@ -259,25 +241,28 @@ impl PlumeQuery {
}
// recursive parser for query string
// allow this clippy lint for now, until someone figures out how to
// refactor this better.
#[allow(clippy::wrong_self_convention)]
fn from_str_req(&mut self, mut query: &str) -> &mut Self {
query = query.trim_left();
if query.is_empty() {
self
} else {
let occur = if query.get(0..1).map(|v| v=="+").unwrap_or(false) {
query = &query[1..];
Occur::Must
} else if query.get(0..1).map(|v| v=="-").unwrap_or(false) {
query = &query[1..];
Occur::MustNot
} else {
Occur::Should
};
gen_parser!(self, query, occur; normal: title, subtitle, content, tag,
instance, author, blog, lang, license;
date: after, before);
self.from_str_req(query)
return self
}
let occur = if query.get(0..1).map(|v| v=="+").unwrap_or(false) {
query = &query[1..];
Occur::Must
} else if query.get(0..1).map(|v| v=="-").unwrap_or(false) {
query = &query[1..];
Occur::MustNot
} else {
Occur::Should
};
gen_parser!(self, query, occur; normal: title, subtitle, content, tag,
instance, author, blog, lang, license;
date: after, before);
self.from_str_req(query)
}
// map a token and it's field to a query
@@ -290,7 +275,7 @@ impl PlumeQuery {
let user_term = Term::from_field_text(field, &token[..pos]);
let instance_term = Term::from_field_text(Searcher::schema().get_field("instance").unwrap(), &token[pos+1..]);
Box::new(BooleanQuery::from(vec![
(Occur::Must, Box::new(TermQuery::new(user_term, if field_name=="author" { IndexRecordOption::Basic }
(Occur::Must, Box::new(TermQuery::new(user_term, if field_name=="author" { IndexRecordOption::Basic }
else { IndexRecordOption::WithFreqsAndPositions }
)) as Box<dyn Query + 'static>),
(Occur::Must, Box::new(TermQuery::new(instance_term, IndexRecordOption::Basic))),
@@ -320,15 +305,34 @@ impl PlumeQuery {
}
}
impl std::str::FromStr for PlumeQuery {
type Err = !;
/// Create a new Query from &str
/// Same as doing
/// ```rust
/// # extern crate plume_models;
/// # use plume_models::search::Query;
/// let mut q = Query::new();
/// q.parse_query("some query");
/// ```
fn from_str(query: &str) -> Result<PlumeQuery, !> {
let mut res: PlumeQuery = Default::default();
res.from_str_req(&query.trim());
Ok(res)
}
}
impl ToString for PlumeQuery {
fn to_string(&self) -> String {
let mut result = String::new();
for (occur, val) in &self.text {
if val.contains(' ') {
result.push_str(&format!("{}\"{}\" ", Self::occur_to_str(&occur), val));
result.push_str(&format!("{}\"{}\" ", Self::occur_to_str(*occur), val));
} else {
result.push_str(&format!("{}{} ", Self::occur_to_str(&occur), val));
result.push_str(&format!("{}{} ", Self::occur_to_str(*occur), val));
}
}
@@ -340,4 +344,3 @@ impl ToString for PlumeQuery {
result
}
}
plume-models/src/search/searcher.rs
+1 -1
View File
@@ -183,7 +183,7 @@ impl Searcher {
let res = searcher.search(&query.into_query(), &collector).unwrap();
res.get(min as usize..).unwrap_or(&[])
.into_iter()
.iter()
.filter_map(|(_,doc_add)| {
let doc = searcher.doc(*doc_add).ok()?;
let id = doc.get_first(post_id)?;
plume-models/src/users.rs
+6 -3
View File
@@ -171,6 +171,9 @@ impl User {
.select(post_authors::post_id)
.load(conn)?;
for post_id in all_their_posts_ids {
// disabling this lint, because otherwise we'd have to turn it on
// the head, and make it even harder to follow!
#[allow(clippy::op_ref)]
let has_other_authors = post_authors::table
.filter(post_authors::post_id.eq(post_id))
.filter(post_authors::author_id.ne(self.id))
@@ -489,7 +492,7 @@ impl User {
Ok(json["items"]
.as_array()
.unwrap_or(&vec![])
.into_iter()
.iter()
.filter_map(|j| serde_json::from_value(j.clone()).ok())
.collect::<Vec<T>>())
}
@@ -512,7 +515,7 @@ impl User {
Ok(json["items"]
.as_array()
.unwrap_or(&vec![])
.into_iter()
.iter()
.filter_map(|j| serde_json::from_value(j.clone()).ok())
.collect::<Vec<String>>())
}
@@ -746,7 +749,7 @@ impl User {
pub fn avatar_url(&self, conn: &Connection) -> String {
self.avatar_id.and_then(|id|
Media::get(conn, id).and_then(|m| m.url(conn)).ok()
).unwrap_or("/static/default-avatar.png".to_string())
).unwrap_or_else(|| "/static/default-avatar.png".to_string())
}
pub fn webfinger(&self, conn: &Connection) -> Result<Webfinger> {