From be20a50acc9a4201c7872708c88d4713cdc3dcd9 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 27 Sep 2023 15:08:39 +0200 Subject: [PATCH] i18n: clippy cleanups --- crates/i18n/src/lib.rs | 5 +- crates/i18n/src/sprintf/argument.rs | 14 ++++-- crates/i18n/src/sprintf/formatter.rs | 68 ++++++++++++++------------ crates/i18n/src/sprintf/message.rs | 73 ++++++++++------------------ crates/i18n/src/sprintf/mod.rs | 7 ++- crates/i18n/src/sprintf/parser.rs | 40 +++++++-------- crates/i18n/src/translations.rs | 20 ++------ 7 files changed, 102 insertions(+), 125 deletions(-) diff --git a/crates/i18n/src/lib.rs b/crates/i18n/src/lib.rs index 1787fa9e4..cc77b11de 100644 --- a/crates/i18n/src/lib.rs +++ b/crates/i18n/src/lib.rs @@ -12,5 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +#![warn(clippy::pedantic)] +#![deny(clippy::all)] + pub mod sprintf; -mod translations; +pub mod translations; diff --git a/crates/i18n/src/sprintf/argument.rs b/crates/i18n/src/sprintf/argument.rs index bcd1288d7..f54772b1c 100644 --- a/crates/i18n/src/sprintf/argument.rs +++ b/crates/i18n/src/sprintf/argument.rs @@ -18,16 +18,18 @@ use serde_json::Value; /// A list of arguments that can be accessed by index or name. #[derive(Debug, Clone, Default)] -pub struct ArgumentList { +pub struct List { arguments: Vec, name_index: HashMap, } -impl ArgumentList { +impl List { + #[must_use] pub fn get_by_index(&self, index: usize) -> Option<&Value> { self.arguments.get(index) } + #[must_use] pub fn get_by_name(&self, name: &str) -> Option<&Value> { self.name_index .get(name) @@ -35,7 +37,7 @@ impl ArgumentList { } } -impl> FromIterator for ArgumentList { +impl> FromIterator for List { fn from_iter>(iter: T) -> Self { let mut arguments = Vec::new(); let mut name_index = HashMap::new(); @@ -87,6 +89,7 @@ impl From<(String, Value)> for Argument { impl Argument { /// Create a new argument with the given name and value. + #[must_use] pub fn named(name: String, value: Value) -> Self { Self { name: Some(name), @@ -95,6 +98,7 @@ impl Argument { } /// Set the name of the argument. + #[must_use] pub fn with_name(mut self, name: String) -> Self { self.name = Some(name); self @@ -109,7 +113,7 @@ mod tests { #[test] fn test_argument_list() { - let list = ArgumentList::from_iter([ + let list = List::from_iter([ ("hello", json!("world")), ("alice", json!(null)), ("bob", json!(42)), @@ -125,7 +129,7 @@ mod tests { assert_eq!(list.get_by_name("bob"), Some(&json!(42))); assert_eq!(list.get_by_name("charlie"), None); - let list = ArgumentList::from_iter([ + let list = List::from_iter([ Argument::from(json!("hello")), Argument::named("alice".to_owned(), json!(null)), Argument::named("bob".to_owned(), json!(42)), diff --git a/crates/i18n/src/sprintf/formatter.rs b/crates/i18n/src/sprintf/formatter.rs index 53cca2603..8b6a2d65e 100644 --- a/crates/i18n/src/sprintf/formatter.rs +++ b/crates/i18n/src/sprintf/formatter.rs @@ -18,7 +18,9 @@ use serde_json::{ser::PrettyFormatter, Value}; use thiserror::Error; use super::{ArgumentList, Message}; -use crate::sprintf::message::{ArgumentReference, MessagePart, Placeholder, TypeSpecifier}; +use crate::sprintf::message::{ + ArgumentReference, PaddingSpecifier, Part, Placeholder, TypeSpecifier, +}; macro_rules! format_placeholder { ($value:expr, $type:literal, $placeholder:expr) => { @@ -185,13 +187,9 @@ fn find_value<'a>( current_index: usize, ) -> Result<&'a Value, FormatError> { match requested_argument { - Some(ArgumentReference::Named(name)) => { - arguments - .get_by_name(name) - .ok_or(FormatError::UnknownNamedArgument { - name: name.to_owned(), - }) - } + Some(ArgumentReference::Named(name)) => arguments + .get_by_name(name) + .ok_or(FormatError::UnknownNamedArgument { name: name.clone() }), Some(ArgumentReference::Indexed(index)) => arguments .get_by_index(*index - 1) @@ -216,11 +214,15 @@ fn to_precision(number: f64, mut placeholder: Placeholder) -> String { } // This tells us how many numbers are before the decimal point - let log10 = number.abs().log10().floor() as i32; + // This lossy cast is fine because we only care about the order of magnitude, + // and special cases are handled above + #[allow(clippy::cast_possible_truncation)] + let log10 = number.abs().log10().floor() as i64; + let precision_i64 = precision.try_into().unwrap_or(i64::MAX); // We can fit the number in the precision, so we just format it as normal - if log10 > 0 && log10 <= precision as i32 || number.abs() < 10.0 { + if log10 > 0 && log10 <= precision_i64 || number.abs() < 10.0 { // We remove the number of digits before the decimal point from the precision - placeholder.precision = Some(precision - 1 - log10.max(0) as usize); + placeholder.precision = Some(precision - 1 - log10.try_into().unwrap_or(0usize)); format_placeholder!(number, &placeholder) } else { // Else in scientific notation there is always one digit before the decimal @@ -230,6 +232,7 @@ fn to_precision(number: f64, mut placeholder: Placeholder) -> String { } } +#[allow(clippy::too_many_lines, clippy::match_same_arms)] fn format_value(value: &Value, placeholder: &Placeholder) -> Result { match (value, &placeholder.type_specifier) { (Value::Number(number), ts @ TypeSpecifier::BinaryNumber) => { @@ -297,10 +300,9 @@ fn format_value(value: &Value, placeholder: &Placeholder) -> Result Result Result Result Result Result { let mut buffer = String::new(); @@ -492,13 +497,13 @@ impl Message { let mut current_placeholder = 0usize; for part in self.parts() { match part { - MessagePart::Percent => { + Part::Percent => { buffer.push('%'); } - MessagePart::Text(text) => { + Part::Text(text) => { buffer.push_str(text); } - MessagePart::Placeholder(placeholder) => { + Part::Placeholder(placeholder) => { let value = find_value( arguments, placeholder.requested_argument.as_ref(), @@ -511,8 +516,7 @@ impl Message { let formatted = if let Some(width) = placeholder.width { let spacer = placeholder .padding_specifier - .map(|p| p.char()) - .unwrap_or(' '); + .map_or(' ', PaddingSpecifier::char); let alignment = if placeholder.left_align { Alignment::Left diff --git a/crates/i18n/src/sprintf/message.rs b/crates/i18n/src/sprintf/message.rs index fab80141c..92a8a2582 100644 --- a/crates/i18n/src/sprintf/message.rs +++ b/crates/i18n/src/sprintf/message.rs @@ -106,7 +106,7 @@ impl std::fmt::Display for TypeSpecifier { Self::HexadecimalNumberUppercase => 'X', Self::Json => 'j', }; - write!(f, "{}", specifier) + write!(f, "{specifier}") } } @@ -119,8 +119,8 @@ pub enum ArgumentReference { impl std::fmt::Display for ArgumentReference { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - ArgumentReference::Indexed(index) => write!(f, "{}$", index), - ArgumentReference::Named(name) => write!(f, "({})", name), + ArgumentReference::Indexed(index) => write!(f, "{index}$"), + ArgumentReference::Named(name) => write!(f, "({name})"), } } } @@ -135,16 +135,16 @@ impl std::fmt::Display for PaddingSpecifier { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { PaddingSpecifier::Zero => write!(f, "0"), - PaddingSpecifier::Char(c) => write!(f, "'{}", c), + PaddingSpecifier::Char(c) => write!(f, "'{c}"), } } } impl PaddingSpecifier { - pub fn char(&self) -> char { + pub fn char(self) -> char { match self { PaddingSpecifier::Zero => '0', - PaddingSpecifier::Char(c) => *c, + PaddingSpecifier::Char(c) => c, } } @@ -154,13 +154,6 @@ impl PaddingSpecifier { PaddingSpecifier::Char(_) => false, } } - - pub const fn is_char(self) -> bool { - match self { - PaddingSpecifier::Zero => false, - PaddingSpecifier::Char(_) => true, - } - } } #[derive(Debug, Clone)] @@ -177,14 +170,7 @@ pub struct Placeholder { impl Placeholder { pub fn padding_specifier_is_zero(&self) -> bool { self.padding_specifier - .map(PaddingSpecifier::is_zero) - .unwrap_or(false) - } - - pub fn padding_specifier_is_char(&self) -> bool { - self.padding_specifier - .map(PaddingSpecifier::is_char) - .unwrap_or(false) + .is_some_and(PaddingSpecifier::is_zero) } /// Whether it should be formatted as a number for the width argument @@ -198,7 +184,7 @@ impl std::fmt::Display for Placeholder { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "%")?; if let Some(argument) = &self.requested_argument { - write!(f, "{}", argument)?; + write!(f, "{argument}")?; } if self.plus_sign { @@ -206,7 +192,7 @@ impl std::fmt::Display for Placeholder { } if let Some(padding_specifier) = &self.padding_specifier { - write!(f, "{}", padding_specifier)?; + write!(f, "{padding_specifier}")?; } if self.left_align { @@ -214,11 +200,11 @@ impl std::fmt::Display for Placeholder { } if let Some(width) = self.width { - write!(f, "{}", width)?; + write!(f, "{width}")?; } if let Some(precision) = self.precision { - write!(f, ".{}", precision)?; + write!(f, ".{precision}")?; } write!(f, "{}", self.type_specifier) @@ -227,37 +213,28 @@ impl std::fmt::Display for Placeholder { #[derive(Debug, Clone)] pub struct Message { - parts: Vec, + parts: Vec, } impl std::fmt::Display for Message { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - for part in self.parts.iter() { - write!(f, "{}", part)?; + for part in &self.parts { + write!(f, "{part}")?; } Ok(()) } } -impl FromIterator for Message { - fn from_iter>(iter: T) -> Self { +impl FromIterator for Message { + fn from_iter>(iter: T) -> Self { Self { parts: iter.into_iter().collect(), } } } -impl IntoIterator for Message { - type Item = MessagePart; - type IntoIter = std::vec::IntoIter; - - fn into_iter(self) -> Self::IntoIter { - self.parts.into_iter() - } -} - impl Message { - pub fn parts(&self) -> std::slice::Iter<'_, MessagePart> { + pub(crate) fn parts(&self) -> std::slice::Iter<'_, Part> { self.parts.iter() } } @@ -272,35 +249,35 @@ impl Serialize for Message { impl<'de> Deserialize<'de> for Message { fn deserialize>(deserializer: D) -> Result { let string = String::deserialize(deserializer)?; - Ok(string.parse().map_err(serde::de::Error::custom)?) + string.parse().map_err(serde::de::Error::custom) } } #[derive(Debug, Clone)] -pub enum MessagePart { +pub(crate) enum Part { Percent, Text(String), Placeholder(Placeholder), } -impl From for MessagePart { +impl From for Part { fn from(placeholder: Placeholder) -> Self { Self::Placeholder(placeholder) } } -impl From for MessagePart { +impl From for Part { fn from(text: String) -> Self { Self::Text(text) } } -impl std::fmt::Display for MessagePart { +impl std::fmt::Display for Part { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - MessagePart::Percent => write!(f, "%%"), - MessagePart::Text(text) => write!(f, "{}", text), - MessagePart::Placeholder(placeholder) => write!(f, "{}", placeholder), + Part::Percent => write!(f, "%%"), + Part::Text(text) => write!(f, "{text}"), + Part::Placeholder(placeholder) => write!(f, "{placeholder}"), } } } diff --git a/crates/i18n/src/sprintf/mod.rs b/crates/i18n/src/sprintf/mod.rs index 65a3506ca..355a3598b 100644 --- a/crates/i18n/src/sprintf/mod.rs +++ b/crates/i18n/src/sprintf/mod.rs @@ -20,11 +20,12 @@ mod parser; use thiserror::Error; pub use self::{ - argument::{Argument, ArgumentList}, + argument::{Argument, List as ArgumentList}, message::Message, }; macro_rules! arg_list_inner { + ($var:ident |) => { }; ($var:ident | $name:ident = $($arg:expr)*, $($rest:tt)*) => {{ $var.push($crate::sprintf::Argument::from((stringify!($name), ::serde_json::json!($($arg)*)))); $crate::sprintf::arg_list_inner!($var | $($rest)* ); @@ -39,12 +40,11 @@ macro_rules! arg_list_inner { ($var:ident | $($arg:expr)*) => {{ $var.push($crate::sprintf::Argument::from(::serde_json::json!($($arg)*))); }}; - ($var:ident |) => { }; } macro_rules! arg_list { ($($args:tt)*) => {{ - let mut __args = Vec::new(); + let mut __args = Vec::<$crate::sprintf::Argument>::new(); $crate::sprintf::arg_list_inner!(__args | $($args)* ); $crate::sprintf::ArgumentList::from_iter(__args) }} @@ -72,7 +72,6 @@ macro_rules! sprintf { pub(crate) use arg_list; pub(crate) use arg_list_inner; -pub(crate) use sprintf; #[derive(Debug, Error)] #[error(transparent)] diff --git a/crates/i18n/src/sprintf/parser.rs b/crates/i18n/src/sprintf/parser.rs index 74a521f08..ed3ea9694 100644 --- a/crates/i18n/src/sprintf/parser.rs +++ b/crates/i18n/src/sprintf/parser.rs @@ -19,7 +19,7 @@ use std::str::FromStr; use pest::{error::ErrorVariant, iterators::Pair, Parser, Span}; use super::message::{ - ArgumentReference, Message, MessagePart, PaddingSpecifier, Placeholder, TypeSpecifier, + ArgumentReference, Message, PaddingSpecifier, Part, Placeholder, TypeSpecifier, }; #[derive(pest_derive::Parser)] @@ -73,17 +73,17 @@ fn ensure_rule_type(pair: &Pair, rule: Rule) -> Result<()> { } } -fn interpret_ident(pair: Pair) -> Result { - ensure_rule_type(&pair, Rule::ident)?; +fn interpret_ident(pair: &Pair) -> Result { + ensure_rule_type(pair, Rule::ident)?; Ok(pair.as_str().to_owned()) } -fn interpret_number(pair: Pair) -> Result { - ensure_rule_type(&pair, Rule::number)?; +fn interpret_number(pair: &Pair) -> Result { + ensure_rule_type(pair, Rule::number)?; pair.as_str().parse().map_err(|e| { Error::new_from_span( ErrorVariant::CustomError { - message: format!("Failed to parse number: {}", e), + message: format!("Failed to parse number: {e}"), }, pair.as_span(), ) @@ -96,7 +96,7 @@ fn interpret_arg_named(pair: Pair) -> Result { let mut pairs = pair.into_inner(); let ident = next_pair(&mut pairs, span)?; - let ident = interpret_ident(ident)?; + let ident = interpret_ident(&ident)?; ensure_end_of_pairs(&mut pairs, span)?; Ok(ArgumentReference::Named(ident)) @@ -108,14 +108,14 @@ fn interpret_arg_indexed(pair: Pair) -> Result { let mut pairs = pair.into_inner(); let number = next_pair(&mut pairs, span)?; - let number = interpret_number(number)?; + let number = interpret_number(&number)?; ensure_end_of_pairs(&mut pairs, span)?; Ok(ArgumentReference::Indexed(number)) } -fn interpret_padding_specifier(pair: Pair) -> Result { - ensure_rule_type(&pair, Rule::padding_specifier)?; +fn interpret_padding_specifier(pair: &Pair) -> Result { + ensure_rule_type(pair, Rule::padding_specifier)?; let specifier: Vec = pair.as_str().chars().collect(); let specifier = match specifier[..] { @@ -124,7 +124,7 @@ fn interpret_padding_specifier(pair: Pair) -> Result { ref specifier => { return Err(Error::new_from_span( ErrorVariant::CustomError { - message: format!("Unexpected padding specifier: {:?}", specifier), + message: format!("Unexpected padding specifier: {specifier:?}"), }, pair.as_span(), )) @@ -140,7 +140,7 @@ fn interpret_width(pair: Pair) -> Result { let mut pairs = pair.into_inner(); let number = next_pair(&mut pairs, span)?; - let number = interpret_number(number)?; + let number = interpret_number(&number)?; ensure_end_of_pairs(&mut pairs, span)?; Ok(number) @@ -152,14 +152,14 @@ fn interpret_precision(pair: Pair) -> Result { let mut pairs = pair.into_inner(); let number = next_pair(&mut pairs, span)?; - let number = interpret_number(number)?; + let number = interpret_number(&number)?; ensure_end_of_pairs(&mut pairs, span)?; Ok(number) } -fn interpret_type_specifier(pair: Pair) -> Result { - ensure_rule_type(&pair, Rule::type_specifier)?; +fn interpret_type_specifier(pair: &Pair) -> Result { + ensure_rule_type(pair, Rule::type_specifier)?; let specifier: Vec = pair.as_str().chars().collect(); let type_specifier = match specifier[..] { @@ -218,7 +218,7 @@ fn interpret_placeholder(pair: Pair) -> Result { }; let padding_specifier = if current_pair.as_rule() == Rule::padding_specifier { - let padding_specifier = interpret_padding_specifier(current_pair)?; + let padding_specifier = interpret_padding_specifier(¤t_pair)?; current_pair = next_pair(&mut pairs, span)?; Some(padding_specifier) } else { @@ -248,7 +248,7 @@ fn interpret_placeholder(pair: Pair) -> Result { None }; - let type_specifier = interpret_type_specifier(current_pair)?; + let type_specifier = interpret_type_specifier(¤t_pair)?; ensure_end_of_pairs(&mut pairs, span)?; @@ -272,7 +272,7 @@ impl FromStr for Message { .filter(|pair| pair.as_rule() != Rule::EOI) .map(|pair| match pair.as_rule() { Rule::text => Ok(pair.as_str().to_owned().into()), - Rule::percent => Ok(MessagePart::Percent), + Rule::percent => Ok(Part::Percent), Rule::placeholder => Ok(interpret_placeholder(pair)?.into()), _ => Err(unexpected_rule_error(&pair)), }) @@ -335,9 +335,9 @@ mod tests { "Hello %(who)s!", ]; - for case in cases.into_iter() { + for case in cases { let result: Result = case.parse(); - assert!(result.is_ok(), "Failed to parse: {}", case); + assert!(result.is_ok(), "Failed to parse: {case}"); let message = result.unwrap(); assert_eq!(message.to_string(), *case); } diff --git a/crates/i18n/src/translations.rs b/crates/i18n/src/translations.rs index 19156ea16..d9277e2e1 100644 --- a/crates/i18n/src/translations.rs +++ b/crates/i18n/src/translations.rs @@ -87,7 +87,7 @@ impl TranslationTree { match self { TranslationTree::Message(_) => None, TranslationTree::Children(tree) => { - let child = tree.get(next.deref())?; + let child = tree.get(&*next)?; child.walk_path(path) } } @@ -96,14 +96,7 @@ impl TranslationTree { fn as_message(&self) -> Option<&Message> { match self { TranslationTree::Message(message) => Some(message), - _ => None, - } - } - - fn as_children(&self) -> Option<&BTreeMap> { - match self { - TranslationTree::Children(children) => Some(children), - _ => None, + TranslationTree::Children(_) => None, } } } @@ -111,7 +104,7 @@ impl TranslationTree { #[cfg(test)] mod tests { use super::*; - use crate::sprintf::arg_list; + use crate::sprintf::{arg_list, ArgumentList}; #[test] fn test_it_works() { @@ -131,15 +124,12 @@ mod tests { let message = tree.message("hello"); assert!(message.is_some()); let message = message.unwrap(); - assert_eq!(message.format(&Default::default()).unwrap(), "world"); + assert_eq!(message.format(&ArgumentList::default()).unwrap(), "world"); let message = tree.message("damals.about_x_hours_ago.one"); assert!(message.is_some()); let message = message.unwrap(); - assert_eq!( - message.format(&Default::default()).unwrap(), - "about one hour ago" - ); + assert_eq!(message.format(&arg_list!()).unwrap(), "about one hour ago"); let message = tree.pluralize("damals.about_x_hours_ago", PluralCategory::Other); assert!(message.is_some());