Minor cleanups and switch to fmt::Display in types

zbraniecki · zbraniecki · commit ad8fbc5441a1 · 2019-07-11T14:46:38.000-07:00
diff --git a/fluent-bundle/src/bundle.rs b/fluent-bundle/src/bundle.rs
@@ -17,7 +17,6 @@ use crate::entry::GetEntry;
 use crate::errors::FluentError;
 use crate::resolve::{resolve_value_for_entry, Scope};
 use crate::resource::FluentResource;
-use crate::types::DisplayableNode;
 use crate::types::FluentValue;
 
 #[derive(Debug, PartialEq)]
@@ -387,25 +386,14 @@ impl<'bundle, R> FluentBundle<'bundle, R> {
                 .attributes
                 .iter()
                 .find(|attr| attr.id.name == attr_name)?;
-            resolve_value_for_entry(
-                &attr.value,
-                DisplayableNode::new(message.id.name, Some(attr.id.name)),
-                &mut scope,
-            )
-            .to_string()
+            resolve_value_for_entry(&attr.value, (message, attr).into(), &mut scope).to_string()
         } else {
             let message_id = path;
             let message = self.get_message(message_id)?;
             message
                 .value
                 .as_ref()
-                .map(|value| {
-                    resolve_value_for_entry(
-                        value,
-                        DisplayableNode::new(message.id.name, None),
-                        &mut scope,
-                    )
-                })?
+                .map(|value| resolve_value_for_entry(value, message.into(), &mut scope))?
                 .to_string()
         };
 
@@ -470,10 +458,10 @@ impl<'bundle, R> FluentBundle<'bundle, R> {
         let mut errors = vec![];
         let message = self.get_message(message_id)?;
 
-        let value = message.value.as_ref().map(|value| {
-            resolve_value_for_entry(value, DisplayableNode::new(message.id.name, None), &mut scope)
-                .to_string()
-        });
+        let value = message
+            .value
+            .as_ref()
+            .map(|value| resolve_value_for_entry(value, message.into(), &mut scope).to_string());
 
         // Setting capacity helps performance for cases with attributes,
         // but is slower than `::new` for cases without.
@@ -485,11 +473,7 @@ impl<'bundle, R> FluentBundle<'bundle, R> {
         };
 
         for attr in message.attributes.iter() {
-            let val = resolve_value_for_entry(
-                &attr.value,
-                DisplayableNode::new(message.id.name, Some(attr.id.name)),
-                &mut scope,
-            );
+            let val = resolve_value_for_entry(&attr.value, (message, attr).into(), &mut scope);
             attributes.insert(attr.id.name, val.to_string());
         }
 
diff --git a/fluent-bundle/src/resolve.rs b/fluent-bundle/src/resolve.rs
@@ -67,11 +67,7 @@ impl<'bundle, R: Borrow<FluentResource>> Scope<'bundle, R> {
         F: FnMut(&mut Scope<'bundle, R>) -> FluentValue<'bundle>,
     {
         let mut hasher = DefaultHasher::new();
-        (entry.id, entry.attribute).hash(&mut hasher);
-        entry.id.hash(&mut hasher);
-        if let Some(attr) = entry.attribute {
-            attr.hash(&mut hasher);
-        }
+        entry.hash(&mut hasher);
         let hash = hasher.finish();
 
         if self.travelled.borrow().contains(&hash) {
@@ -109,7 +105,9 @@ where
     if value.elements.len() == 1 {
         return match value.elements[0] {
             ast::PatternElement::TextElement(s) => FluentValue::String(s.into()),
-            ast::PatternElement::Placeable(ref p) => scope.track(entry.clone(), |scope| p.resolve(scope)),
+            ast::PatternElement::Placeable(ref p) => {
+                scope.track(entry.clone(), |scope| p.resolve(scope))
+            }
         };
     }
 
@@ -135,7 +133,9 @@ fn generate_ref_error<'source, R>(
 where
     R: Borrow<FluentResource>,
 {
-    scope.errors.push(ResolverError::Reference(node.get_error()));
+    scope
+        .errors
+        .push(ResolverError::Reference(node.get_error()));
     FluentValue::Error(node)
 }
 
@@ -243,7 +243,8 @@ impl<'source> ResolveValue<'source> for ast::InlineExpression<'source> {
 
                 if let Some(msg) = msg {
                     if let Some(attr) = attribute {
-                        scope.maybe_resolve_attribute(&msg.attributes, self.into(), attr.name)
+                        scope
+                            .maybe_resolve_attribute(&msg.attributes, self.into(), attr.name)
                             .unwrap_or_else(|| generate_ref_error(scope, self.into()))
                     } else if let Some(value) = msg.value.as_ref() {
                         scope.track(self.into(), |scope| value.resolve(scope))
@@ -268,7 +269,8 @@ impl<'source> ResolveValue<'source> for ast::InlineExpression<'source> {
 
                 let value = if let Some(term) = term {
                     if let Some(attr) = attribute {
-                        scope.maybe_resolve_attribute(&term.attributes, self.into(), attr.name)
+                        scope
+                            .maybe_resolve_attribute(&term.attributes, self.into(), attr.name)
                             .unwrap_or_else(|| generate_ref_error(scope, self.into()))
                     } else {
                         term.resolve(&mut scope)
@@ -280,7 +282,8 @@ impl<'source> ResolveValue<'source> for ast::InlineExpression<'source> {
                 value
             }
             ast::InlineExpression::FunctionReference { id, arguments } => {
-                let (resolved_positional_args, resolved_named_args) = get_arguments(scope, arguments);
+                let (resolved_positional_args, resolved_named_args) =
+                    get_arguments(scope, arguments);
 
                 let func = scope.bundle.get_function(id.name);
 
@@ -301,7 +304,8 @@ impl<'source> ResolveValue<'source> for ast::InlineExpression<'source> {
                 } else {
                     let displayable_node: DisplayableNode = self.into();
                     if scope.local_args.is_none() {
-                        scope.errors
+                        scope
+                            .errors
                             .push(ResolverError::Reference(displayable_node.get_error()));
                     }
                     FluentValue::Error(displayable_node)
diff --git a/fluent-bundle/src/types.rs b/fluent-bundle/src/types.rs
@@ -11,8 +11,8 @@
 //! [`resolve`]: ../resolve
 //! [`FluentBundle::format`]: ../bundle/struct.FluentBundle.html#method.format
 
-use std::borrow::Borrow;
-use std::borrow::Cow;
+use std::borrow::{Borrow, Cow};
+use std::fmt;
 use std::str::FromStr;
 
 use fluent_syntax::ast;
@@ -21,52 +21,65 @@ use intl_pluralrules::PluralCategory;
 use crate::resolve::Scope;
 use crate::resource::FluentResource;
 
-#[derive(Debug, PartialEq, Clone)]
+#[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)]
 pub enum DisplayableNodeType {
     Message,
     Term,
     Variable,
     Function,
 }
 
-#[derive(Debug, PartialEq, Clone)]
+#[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)]
 pub struct DisplayableNode<'source> {
-    pub node_type: DisplayableNodeType,
-    pub id: &'source str,
-    pub attribute: Option<&'source str>,
+    node_type: DisplayableNodeType,
+    id: &'source str,
+    attribute: Option<&'source str>,
 }
 
 impl<'source> DisplayableNode<'source> {
-    pub fn display(&self) -> String {
-        let mut id = self.id.to_owned();
-        if let Some(attr) = self.attribute {
-            id.push_str(".");
-            id.push_str(attr);
-        }
+    pub fn get_error(&self) -> String {
         match self.node_type {
-            DisplayableNodeType::Message => id,
-            DisplayableNodeType::Term => format!("-{}", id),
-            DisplayableNodeType::Variable => format!("${}", id),
-            DisplayableNodeType::Function => format!("{}()", id),
+            DisplayableNodeType::Message => format!("Unknown message: {}", self),
+            DisplayableNodeType::Term => format!("Unknown term: {}", self),
+            DisplayableNodeType::Variable => format!("Unknown variable: {}", self),
+            DisplayableNodeType::Function => format!("Unknown function: {}", self),
         }
     }
+}
 
-    pub fn get_error(&self) -> String {
-        let mut id = match self.node_type {
-            DisplayableNodeType::Message => String::from("Unknown message: "),
-            DisplayableNodeType::Term => String::from("Unknown term: "),
-            DisplayableNodeType::Variable => String::from("Unknown variable: "),
-            DisplayableNodeType::Function => String::from("Unknown function: "),
+impl<'source> fmt::Display for DisplayableNode<'source> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self.node_type {
+            DisplayableNodeType::Message => write!(f, "{}", self.id)?,
+            DisplayableNodeType::Term => write!(f, "-{}", self.id)?,
+            DisplayableNodeType::Variable => write!(f, "${}", self.id)?,
+            DisplayableNodeType::Function => write!(f, "{}()", self.id)?,
         };
-        id.push_str(&self.display());
-        id
+        if let Some(attr) = self.attribute {
+            write!(f, ".{}", attr)?;
+        }
+        Ok(())
+    }
+}
+
+impl<'source> From<&ast::Message<'source>> for DisplayableNode<'source> {
+    fn from(msg: &ast::Message<'source>) -> Self {
+        DisplayableNode {
+            node_type: DisplayableNodeType::Message,
+            id: msg.id.name,
+            attribute: None,
+        }
     }
+}
 
-    pub fn new(id: &'source str, attribute: Option<&'source str>) -> Self {
+impl<'source> From<(&ast::Message<'source>, &ast::Attribute<'source>)>
+    for DisplayableNode<'source>
+{
+    fn from(input: (&ast::Message<'source>, &ast::Attribute<'source>)) -> Self {
         DisplayableNode {
             node_type: DisplayableNodeType::Message,
-            id,
-            attribute,
+            id: input.0.id.name,
+            attribute: Some(input.1.id.name),
         }
     }
 }
@@ -121,13 +134,18 @@ pub enum FluentValue<'source> {
 
 impl<'source> FluentValue<'source> {
     pub fn into_number<S: ToString>(v: S) -> Self {
-        match f64::from_str(&v.to_string()) {
-            Ok(_) => FluentValue::Number(v.to_string().into()),
-            Err(_) => FluentValue::String(v.to_string().into()),
+        let s = v.to_string();
+        match f64::from_str(&s) {
+            Ok(_) => FluentValue::Number(s.into()),
+            Err(_) => FluentValue::String(s.into()),
         }
     }
 
-    pub fn matches<R: Borrow<FluentResource>>(&self, other: &FluentValue, scope: &Scope<R>) -> bool {
+    pub fn matches<R: Borrow<FluentResource>>(
+        &self,
+        other: &FluentValue,
+        scope: &Scope<R>,
+    ) -> bool {
         match (self, other) {
             (&FluentValue::String(ref a), &FluentValue::String(ref b)) => a == b,
             (&FluentValue::Number(ref a), &FluentValue::Number(ref b)) => a == b,
@@ -152,21 +170,26 @@ impl<'source> FluentValue<'source> {
         match self {
             FluentValue::String(s) => s.clone(),
             FluentValue::Number(n) => n.clone(),
-            FluentValue::Error(d) => d.display().into(),
-            FluentValue::None() => String::from("???").into(),
+            FluentValue::Error(d) => d.to_string().into(),
+            FluentValue::None() => "???".into(),
         }
     }
 }
 
-impl<'source> From<String> for FluentValue<'source> {
-    fn from(s: String) -> Self {
-        FluentValue::String(s.into())
+impl<'source> fmt::Display for FluentValue<'source> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            FluentValue::String(s) => write!(f, "{}", s),
+            FluentValue::Number(n) => write!(f, "{}", n),
+            FluentValue::Error(d) => write!(f, "{}", d),
+            FluentValue::None() => write!(f, "???"),
+        }
     }
 }
 
-impl<'source> From<&'source String> for FluentValue<'source> {
-    fn from(s: &'source String) -> Self {
-        FluentValue::String((&s[..]).into())
+impl<'source> From<String> for FluentValue<'source> {
+    fn from(s: String) -> Self {
+        FluentValue::String(s.into())
     }
 }
 
