refactor!(core): update AttributeUpdate methods returns (#237)

* update AttributeUpdate sigs & default impl

* update merge impls

* add split fallback method

* fix errors

* fix doc

honeycomb-core/src/attributes/collections.rs

@@ -6,10 +6,7 @@
 // ------ IMPORTS
 
 use super::{AttributeBind, AttributeStorage, AttributeUpdate, UnknownAttributeStorage};
-use crate::{
-    cmap::{CMapError, CMapResult},
-    prelude::DartIdType,
-};
+use crate::{cmap::CMapResult, prelude::DartIdType};
 use num_traits::ToPrimitive;
 use stm::{atomically, StmResult, TVar, Transaction};
 
@@ -90,17 +87,17 @@ impl<A: AttributeBind + AttributeUpdate> UnknownAttributeStorage for AttrSparseV
             self.data[lhs_inp as usize].read(trans)?,
             self.data[rhs_inp as usize].read(trans)?,
         ) {
-            (Some(v1), Some(v2)) => Some(AttributeUpdate::merge(v1, v2)),
-            (Some(v), None) | (None, Some(v)) => Some(AttributeUpdate::merge_incomplete(v)),
+            (Some(v1), Some(v2)) => Ok(AttributeUpdate::merge(v1, v2)),
+            (Some(v), None) | (None, Some(v)) => AttributeUpdate::merge_incomplete(v),
             (None, None) => AttributeUpdate::merge_from_none(),
         };
-        if new_v.is_none() {
+        if new_v.is_err() {
             eprintln!("W: cannot merge two null attribute value");
             eprintln!("   setting new target value to `None`");
         }
         self.data[rhs_inp as usize].write(trans, None)?;
         self.data[lhs_inp as usize].write(trans, None)?;
-        self.data[out as usize].write(trans, new_v)?;
+        self.data[out as usize].write(trans, new_v.ok())?;
         Ok(())
     }
 
@@ -115,25 +112,13 @@ impl<A: AttributeBind + AttributeUpdate> UnknownAttributeStorage for AttrSparseV
             self.data[lhs_inp as usize].read(trans)?,
             self.data[rhs_inp as usize].read(trans)?,
         ) {
-            (Some(v1), Some(v2)) => Some(AttributeUpdate::merge(v1, v2)),
-            (Some(_), None) | (None, Some(_)) => {
-                return Err(CMapError::FailedAttributeMerge(
-                    "missing one value for merge",
-                ))
-            }
-            (None, None) => {
-                return Err(CMapError::FailedAttributeMerge(
-                    "missing both values for merge",
-                ))
-            }
+            (Some(v1), Some(v2)) => AttributeUpdate::merge(v1, v2),
+            (Some(v), None) | (None, Some(v)) => AttributeUpdate::merge_incomplete(v)?,
+            (None, None) => AttributeUpdate::merge_from_none()?,
         };
-        if new_v.is_none() {
-            eprintln!("W: cannot merge two null attribute value");
-            eprintln!("   setting new target value to `None`");
-        }
         self.data[rhs_inp as usize].write(trans, None)?;
         self.data[lhs_inp as usize].write(trans, None)?;
-        self.data[out as usize].write(trans, new_v)?;
+        self.data[out as usize].write(trans, Some(new_v))?;
         Ok(())
     }
 
@@ -144,8 +129,12 @@ impl<A: AttributeBind + AttributeUpdate> UnknownAttributeStorage for AttrSparseV
         rhs_out: DartIdType,
         inp: DartIdType,
     ) -> StmResult<()> {
-        if let Some(val) = self.data[inp as usize].read(trans)? {
-            let (lhs_val, rhs_val) = AttributeUpdate::split(val);
+        let res = if let Some(val) = self.data[inp as usize].read(trans)? {
+            Ok(AttributeUpdate::split(val))
+        } else {
+            AttributeUpdate::split_from_none()
+        };
+        if let Ok((lhs_val, rhs_val)) = res {
             self.data[inp as usize].write(trans, None)?;
             self.data[lhs_out as usize].write(trans, Some(lhs_val))?;
             self.data[rhs_out as usize].write(trans, Some(rhs_val))?;
@@ -165,14 +154,14 @@ impl<A: AttributeBind + AttributeUpdate> UnknownAttributeStorage for AttrSparseV
         rhs_out: DartIdType,
         inp: DartIdType,
     ) -> CMapResult<()> {
-        if let Some(val) = self.data[inp as usize].read(trans)? {
-            let (lhs_val, rhs_val) = AttributeUpdate::split(val);
-            self.data[inp as usize].write(trans, None)?;
-            self.data[lhs_out as usize].write(trans, Some(lhs_val))?;
-            self.data[rhs_out as usize].write(trans, Some(rhs_val))?;
+        let (lhs_val, rhs_val) = if let Some(val) = self.data[inp as usize].read(trans)? {
+            AttributeUpdate::split(val)
         } else {
-            return Err(CMapError::FailedAttributeSplit("no value to split from"));
-        }
+            AttributeUpdate::split_from_none()?
+        };
+        self.data[inp as usize].write(trans, None)?;
+        self.data[lhs_out as usize].write(trans, Some(lhs_val))?;
+        self.data[rhs_out as usize].write(trans, Some(rhs_val))?;
         Ok(())
     }
 }

honeycomb-core/src/attributes/tests.rs

@@ -8,7 +8,7 @@ use super::{
     UnknownAttributeStorage,
 };
 use crate::{
-    cmap::EdgeIdType,
+    cmap::{CMapResult, EdgeIdType},
     prelude::{CMap2, CMapBuilder, FaceIdType, OrbitPolicy, Vertex2, VertexIdType},
 };
 use std::any::Any;
@@ -35,12 +35,12 @@ impl AttributeUpdate for Temperature {
         (attr, attr)
     }
 
-    fn merge_incomplete(attr: Self) -> Self {
-        Temperature::from(attr.val / 2.0)
+    fn merge_incomplete(attr: Self) -> CMapResult<Self> {
+        Ok(Temperature::from(attr.val / 2.0))
     }
 
-    fn merge_from_none() -> Option<Self> {
-        Some(Temperature::from(0.0))
+    fn merge_from_none() -> CMapResult<Self> {
+        Ok(Temperature::from(0.0))
     }
 }
 
@@ -449,9 +449,9 @@ fn attribute_update() {
     assert_eq!(Temperature::split(t_new), (t_ref, t_ref)); // or Temperature::_
     assert_eq!(
         Temperature::merge_incomplete(t_ref),
-        Temperature::from(t_ref.val / 2.0)
+        Ok(Temperature::from(t_ref.val / 2.0))
     );
-    assert_eq!(Temperature::merge_from_none(), Some(Temperature::from(0.0)));
+    assert_eq!(Temperature::merge_from_none(), Ok(Temperature::from(0.0)));
 }
 
 #[test]

honeycomb-core/src/attributes/traits.rs

@@ -6,11 +6,11 @@
 // ------ IMPORTS
 
 use crate::{
-    cmap::CMapResult,
+    cmap::{CMapError, CMapResult},
     prelude::{DartIdType, OrbitPolicy},
 };
 use downcast_rs::{impl_downcast, Downcast};
-use std::any::Any;
+use std::any::{type_name, Any};
 use std::fmt::Debug;
 use stm::{atomically, StmResult, Transaction};
 
@@ -27,7 +27,7 @@ use stm::{atomically, StmResult, Transaction};
 /// like this:
 ///
 /// ```rust
-/// use honeycomb_core::prelude::AttributeUpdate;
+/// use honeycomb_core::prelude::{AttributeUpdate, CMapResult};
 ///
 /// #[derive(Clone, Copy, Debug, PartialEq)]
 /// pub struct Temperature {
@@ -43,12 +43,12 @@ use stm::{atomically, StmResult, Transaction};
 ///         (attr, attr)
 ///     }
 ///
-///     fn merge_incomplete(attr: Self) -> Self {
-///         Temperature { val: attr.val / 2.0 }
+///     fn merge_incomplete(attr: Self) -> CMapResult<Self> {
+///         Ok(Temperature { val: attr.val / 2.0 })
 ///     }
 ///
-///     fn merge_from_none() -> Option<Self> {
-///         Some(Temperature { val: 0.0 })
+///     fn merge_from_none() -> CMapResult<Self> {
+///         Ok(Temperature { val: 0.0 })
 ///     }
 /// }
 ///
@@ -70,18 +70,57 @@ pub trait AttributeUpdate: Sized + Send + Sync + Clone + Copy {
     /// Fallback merging routine, i.e. how to obtain the new attribute value from a single existing
     /// value.
     ///
+    /// The returned value directly affects the behavior of [`UnknownAttributeStorage::merge`],
+    /// [`UnknownAttributeStorage::try_merge`], therefore of sewing methods too.
+    ///
+    /// For example, if this method returns an error for a given attribute, the `try_merge` method
+    /// will fail. This allow the user to define some attributes as essential (fail if the merge
+    /// isn't done properly from two values) and other as mores flexible (can fallback to a default
+    /// value).
+    ///
+    /// # Errors
+    ///
     /// The default implementation simply returns the passed value.
-    fn merge_incomplete(attr: Self) -> Self {
-        attr
+    fn merge_incomplete(attr: Self) -> CMapResult<Self> {
+        Ok(attr)
     }
 
     /// Fallback merging routine, i.e. how to obtain the new attribute value from no existing
     /// value.
     ///
-    /// The default implementation return `None`.
+    /// The returned value directly affects the behavior of [`UnknownAttributeStorage::merge`],
+    /// [`UnknownAttributeStorage::try_merge`], therefore of sewing methods too.
+    ///
+    /// For example, if this method returns an error for a given attribute, the `try_merge` method
+    /// will fail. This allow the user to define some attributes as essential (fail if the merge
+    /// isn't done properly from two values) and others as more flexible (can fallback to a default
+    /// value).
+    ///
+    /// # Errors
+    ///
+    /// The default implementation return `Err(CMapError::FailedAttributeMerge)`.
+    #[allow(clippy::must_use_candidate)]
+    fn merge_from_none() -> CMapResult<Self> {
+        Err(CMapError::FailedAttributeMerge(type_name::<Self>()))
+    }
+
+    /// Fallback splitting routine, i.e. how to obtain the new attribute value from no existing
+    /// value.
+    ///
+    /// The returned value directly affects the behavior of [`UnknownAttributeStorage::split`],
+    /// [`UnknownAttributeStorage::try_split`], therefore of sewing methods too.
+    ///
+    /// For example, if this method returns an error for a given attribute, the `try_split` method
+    /// will fail. This allow the user to define some attributes as essential (fail if the split
+    /// isn't done properly from a value) and others as more flexible (can fallback to a default
+    /// value).
+    ///
+    /// # Errors
+    ///
+    /// The default implementation return `Err(CMapError::FailedAttributeSplit)`.
     #[allow(clippy::must_use_candidate)]
-    fn merge_from_none() -> Option<Self> {
-        None
+    fn split_from_none() -> CMapResult<(Self, Self)> {
+        Err(CMapError::FailedAttributeSplit(type_name::<Self>()))
     }
 }
 
@@ -96,7 +135,7 @@ pub trait AttributeUpdate: Sized + Send + Sync + Clone + Copy {
 /// to faces if we're modeling a 2D mesh:
 ///
 /// ```rust
-/// use honeycomb_core::prelude::{AttributeBind, AttributeUpdate, FaceIdType, OrbitPolicy};
+/// use honeycomb_core::prelude::{AttributeBind, AttributeUpdate, CMapResult, FaceIdType, OrbitPolicy};
 /// use honeycomb_core::attributes::AttrSparseVec;
 ///
 /// #[derive(Clone, Copy, Debug, PartialEq)]
@@ -112,12 +151,12 @@ pub trait AttributeUpdate: Sized + Send + Sync + Clone + Copy {
 /// #         (attr, attr)
 /// #     }
 /// #
-/// #     fn merge_incomplete(attr: Self) -> Self {
-/// #         Temperature { val: attr.val / 2.0 }
+/// #     fn merge_incomplete(attr: Self) -> CMapResult<Self> {
+/// #         Ok(Temperature { val: attr.val / 2.0 })
 /// #     }
 /// #
-/// #     fn merge_from_none() -> Option<Self> {
-/// #         Some(Temperature { val: 0.0 })
+/// #     fn merge_from_none() -> CMapResult<Self> {
+/// #         Ok(Temperature { val: 0.0 })
 /// #     }
 /// # }
 ///

honeycomb-core/src/cmap/error.rs

@@ -6,7 +6,7 @@ use stm::StmError;
 pub type CMapResult<T> = Result<T, CMapError>;
 
 /// `CMap` error enum.
-#[derive(Debug, thiserror::Error)]
+#[derive(Debug, thiserror::Error, PartialEq, Eq)]
 pub enum CMapError {
     /// STM transaction failed.
     #[error("transaction failed")]

honeycomb-core/src/prelude.rs

@@ -2,9 +2,9 @@
 
 pub use crate::attributes::{AttributeBind, AttributeUpdate};
 pub use crate::cmap::{
-    BuilderError, CMap2, CMapBuilder, DartIdType, EdgeIdType, FaceIdType, Orbit2, OrbitPolicy,
-    VertexIdType, VolumeIdType, NULL_DART_ID, NULL_EDGE_ID, NULL_FACE_ID, NULL_VERTEX_ID,
-    NULL_VOLUME_ID,
+    BuilderError, CMap2, CMapBuilder, CMapResult, DartIdType, EdgeIdType, FaceIdType, Orbit2,
+    OrbitPolicy, VertexIdType, VolumeIdType, NULL_DART_ID, NULL_EDGE_ID, NULL_FACE_ID,
+    NULL_VERTEX_ID, NULL_VOLUME_ID,
 };
 pub use crate::geometry::{CoordsError, CoordsFloat, Vector2, Vertex2};
 

honeycomb-kernels/src/grisubal/model.rs

@@ -8,6 +8,7 @@
 
 use crate::grisubal::GrisubalError;
 use honeycomb_core::attributes::AttrSparseVec;
+use honeycomb_core::cmap::CMapResult;
 use honeycomb_core::prelude::{
     AttributeBind, AttributeUpdate, CoordsFloat, DartIdType, OrbitPolicy, Vertex2,
 };
@@ -263,8 +264,8 @@ impl AttributeUpdate for Boundary {
         unreachable!()
     }
 
-    fn merge_from_none() -> Option<Self> {
-        Some(Boundary::None)
+    fn merge_from_none() -> CMapResult<Self> {
+        Ok(Boundary::None)
     }
 }