refactor!: update various return types for better consistency (#67)

* refactor: change Vector2::normal_dir return type to a result

it is a propagation of the internal unit_dir return

* refactor: change CMap2::vertex return type

also clarify documentation of vertex & insert_vertex methods

* refactor: change CMap2::vertex return type to a result

* fix: update cmap2 test code

* fix: update render code

* fix: finish tests updates

* fix: update benchmarks code

* chore: add comments to justify the new unwraps

* refactor: remove the dead / unused code mention around beta render

* doc: fix duplicated line

* chore: correct inconsistent & wrong doc

* chore: update changelog with last PRs' content

Author: imrn99

CHANGELOG.md

@@ -4,7 +4,22 @@
 
 ## To be released
 
-...
+### Workspace
+
+- implement nighly conditional compilation for doc generation (#66)
+
+### Refactor
+
+#### honeycomb-core
+
+<sup>core definitions and tools for combinatorial map implementation</sup>
+
+- change some main methods return type to have better overall consistency (#67):
+    - `Vector2::normal_dir` now returns a result instead of potentially panicking
+    - `CMap2::vertex` now returns a result instead of panicking if no associated vertex is found
+- remove deprecated items from the last release (#64):
+    - `AttrSparseVec::get_mut`, `AttrCompactVec::get_mut`
+    - `utils::square_cmap2`, `utils::splitsquare_cmap2`
 
 ---
 

honeycomb-benches/benches/splitsquaremap/shift.rs

@@ -32,7 +32,9 @@ fn offset(mut map: CMap2<FloatType>, offsets: &[Vector2<FloatType>]) {
     let n_offsets = offsets.len();
     let vertices = map.fetch_vertices();
     vertices.identifiers.iter().for_each(|vertex_id| {
-        let current_value = map.vertex(*vertex_id as DartIdentifier);
+        // we can unwrap this safely since we built the grid manually
+        // & know that vertices are correctly defined
+        let current_value = map.vertex(*vertex_id as DartIdentifier).unwrap();
         let _ = map.replace_vertex(
             *vertex_id as VertexIdentifier,
             current_value + offsets[*vertex_id as usize % n_offsets],
@@ -56,7 +58,9 @@ fn offset_if_inner(mut map: CMap2<FloatType>, offsets: &[Vector2<FloatType>]) {
         }
     });
     inner.iter().for_each(|vertex_id| {
-        let current_value = map.vertex(*vertex_id);
+        // we can unwrap this safely since we built the grid manually
+        // & know that vertices are correctly defined
+        let current_value = map.vertex(*vertex_id).unwrap();
         let _ = map.replace_vertex(
             *vertex_id,
             current_value + offsets[*vertex_id as usize % n_offsets],

honeycomb-benches/benches/squaremap/shift.rs

@@ -32,7 +32,9 @@ fn offset(mut map: CMap2<FloatType>, offsets: &[Vector2<FloatType>]) {
     let n_offsets = offsets.len();
     let vertices = map.fetch_vertices();
     vertices.identifiers.iter().for_each(|vertex_id| {
-        let current_value = map.vertex(*vertex_id as DartIdentifier);
+        // we can unwrap this safely since we built the grid manually
+        // & know that vertices are correctly defined
+        let current_value = map.vertex(*vertex_id as DartIdentifier).unwrap();
         let _ = map.replace_vertex(
             *vertex_id,
             current_value + offsets[*vertex_id as usize % n_offsets],
@@ -56,7 +58,9 @@ fn offset_if_inner(mut map: CMap2<FloatType>, offsets: &[Vector2<FloatType>]) {
         }
     });
     inner.iter().for_each(|vertex_id| {
-        let current_value = map.vertex(*vertex_id);
+        // we can unwrap this safely since we built the grid manually
+        // & know that vertices are correctly defined
+        let current_value = map.vertex(*vertex_id).unwrap();
         let _ = map.replace_vertex(
             *vertex_id,
             current_value + offsets[*vertex_id as usize % n_offsets],

honeycomb-core/src/cmap2/embed.rs

@@ -18,24 +18,30 @@ impl<T: CoordsFloat> CMap2<T> {
         self.vertices.n_attributes()
     }
 
+    #[allow(clippy::missing_errors_doc)]
     /// Fetch vertex value associated to a given identifier.
     ///
     /// # Arguments
     ///
     /// - `vertex_id: VertexIdentifier` -- Identifier of the given vertex.
     ///
-    /// # Return
+    /// # Return / Errors
     ///
-    /// Return a reference to the [Vertex2] associated to the ID.
+    /// This method return a `Result` taking the following values:
+    /// - `Ok(v: Vertex2)` if there is a vertex associated to this ID.
+    /// - `Err(CMapError::UndefinedVertexID)` -- otherwise
     ///
     /// # Panics
     ///
-    /// The method may panic if no vertex is associated to the specified index, or the ID lands
-    /// out of bounds.
+    /// The method may panic if:
+    /// - the index lands out of bounds
+    /// - the index cannot be converted to `usize`
     ///
-    #[must_use = "returned value is not used, consider removing this method call"]
-    pub fn vertex(&self, vertex_id: VertexIdentifier) -> Vertex2<T> {
-        self.vertices.get(&vertex_id).unwrap()
+    pub fn vertex(&self, vertex_id: VertexIdentifier) -> Result<Vertex2<T>, CMapError> {
+        if let Some(val) = self.vertices.get(&vertex_id) {
+            return Ok(*val);
+        }
+        Err(CMapError::UndefinedVertex)
     }
 
     /// Insert a vertex in the combinatorial map.
@@ -49,9 +55,12 @@ impl<T: CoordsFloat> CMap2<T> {
     /// - `vertex_id: VertexIdentifier` -- Vertex identifier to attribute a value to.
     /// - `vertex: impl Into<Vertex2>` -- Value used to create a [Vertex2] value.
     ///
-    /// # Return
+    /// # Panics
     ///
-    /// Return an option which may contain the previous value associated to the specified vertex ID.
+    /// The method may panic if:
+    /// - **there is already a vertex associated to the specified index**
+    /// - the index lands out of bounds
+    /// - the index cannot be converted to `usize`
     ///
     pub fn insert_vertex(&mut self, vertex_id: VertexIdentifier, vertex: impl Into<Vertex2<T>>) {
         self.vertices.insert(&vertex_id, vertex.into());

honeycomb-core/src/cmap2/structure.rs

@@ -133,7 +133,7 @@ use std::collections::BTreeSet;
 /// assert_eq!(&faces.identifiers, &[1]);
 /// // we can check the vertices
 /// let vertices = map.fetch_vertices();
-/// let mut value_iterator = vertices.identifiers.iter().map(|vertex_id| map.vertex(*vertex_id));
+/// let mut value_iterator = vertices.identifiers.iter().map(|vertex_id| map.vertex(*vertex_id).unwrap());
 /// assert_eq!(value_iterator.next(), Some(Vertex2::from((0.0, 0.0)))); // vertex ID 1
 /// assert_eq!(value_iterator.next(), Some(Vertex2::from((0.0, 1.0)))); // vertex ID 3
 /// assert_eq!(value_iterator.next(), Some(Vertex2::from((1.0, 0.0)))); // vertex ID 5

honeycomb-core/src/cmap2/tests.rs

@@ -50,10 +50,10 @@ fn example_test() {
     assert_eq!(map.beta::<2>(2), 4);
     assert_eq!(map.vertex_id(2), 2);
     assert_eq!(map.vertex_id(5), 2);
-    assert_eq!(map.vertex(2), Vertex2::from((1.5, 0.0)));
+    assert_eq!(map.vertex(2).unwrap(), Vertex2::from((1.5, 0.0)));
     assert_eq!(map.vertex_id(3), 3);
     assert_eq!(map.vertex_id(4), 3);
-    assert_eq!(map.vertex(3), Vertex2::from((0.0, 1.5)));
+    assert_eq!(map.vertex(3).unwrap(), Vertex2::from((0.0, 1.5)));
     let edges = map.fetch_edges();
     assert_eq!(&edges.identifiers, &[1, 2, 3, 5, 6]);
 
@@ -62,12 +62,12 @@ fn example_test() {
         map.replace_vertex(2, Vertex2::from((1.0, 0.0))),
         Ok(Vertex2::from((1.5, 0.0)))
     );
-    assert_eq!(map.vertex(2), Vertex2::from((1.0, 0.0)));
+    assert_eq!(map.vertex(2).unwrap(), Vertex2::from((1.0, 0.0)));
     assert_eq!(
         map.replace_vertex(3, Vertex2::from((0.0, 1.0))),
         Ok(Vertex2::from((0.0, 1.5)))
     );
-    assert_eq!(map.vertex(3), Vertex2::from((0.0, 1.0)));
+    assert_eq!(map.vertex(3).unwrap(), Vertex2::from((0.0, 1.0)));
 
     // separate the diagonal from the rest
     map.one_unsew(1);
@@ -89,10 +89,10 @@ fn example_test() {
     assert_eq!(&edges.identifiers, &[1, 3, 5, 6]);
     let vertices = map.fetch_vertices();
     assert_eq!(&vertices.identifiers, &[1, 3, 5, 6]);
-    assert_eq!(map.vertex(1), Vertex2::from((0.0, 0.0)));
-    assert_eq!(map.vertex(5), Vertex2::from((1.0, 0.0)));
-    assert_eq!(map.vertex(6), Vertex2::from((1.0, 1.0)));
-    assert_eq!(map.vertex(3), Vertex2::from((0.0, 1.0)));
+    assert_eq!(map.vertex(1).unwrap(), Vertex2::from((0.0, 0.0)));
+    assert_eq!(map.vertex(5).unwrap(), Vertex2::from((1.0, 0.0)));
+    assert_eq!(map.vertex(6).unwrap(), Vertex2::from((1.0, 1.0)));
+    assert_eq!(map.vertex(3).unwrap(), Vertex2::from((0.0, 1.0)));
     // darts
     assert_eq!(map.n_unused_darts(), 2); // there are unused darts since we removed the diagonal
     assert_eq!(map.beta_runtime(1, 1), 5);
@@ -134,8 +134,8 @@ fn two_sew_complete() {
     map.insert_vertex(3, (1.0, 1.0));
     map.insert_vertex(4, (1.0, 0.0));
     map.two_sew(1, 3);
-    assert_eq!(map.vertex(1), Vertex2::from((0.5, 0.0)));
-    assert_eq!(map.vertex(2), Vertex2::from((0.5, 1.0)));
+    assert_eq!(map.vertex(1).unwrap(), Vertex2::from((0.5, 0.0)));
+    assert_eq!(map.vertex(2).unwrap(), Vertex2::from((0.5, 1.0)));
 }
 
 #[test]
@@ -147,15 +147,15 @@ fn two_sew_incomplete() {
     map.insert_vertex(3, (1.0, 1.0));
     map.two_sew(1, 3);
     // missing beta1 image for dart 3
-    assert_eq!(map.vertex(1), Vertex2::from((0.0, 0.0)));
-    assert_eq!(map.vertex(2), Vertex2::from((0.5, 1.0)));
+    assert_eq!(map.vertex(1).unwrap(), Vertex2::from((0.0, 0.0)));
+    assert_eq!(map.vertex(2).unwrap(), Vertex2::from((0.5, 1.0)));
     map.two_unsew(1);
     assert_eq!(map.add_free_dart(), 4);
     map.one_link(3, 4);
     map.two_sew(1, 3);
     // missing vertex for dart 4
-    assert_eq!(map.vertex(1), Vertex2::from((0.0, 0.0)));
-    assert_eq!(map.vertex(2), Vertex2::from((0.5, 1.0)));
+    assert_eq!(map.vertex(1).unwrap(), Vertex2::from((0.0, 0.0)));
+    assert_eq!(map.vertex(2).unwrap(), Vertex2::from((0.5, 1.0)));
 }
 
 #[test]
@@ -164,8 +164,8 @@ fn two_sew_no_b1() {
     map.insert_vertex(1, (0.0, 0.0));
     map.insert_vertex(2, (1.0, 1.0));
     map.two_sew(1, 2);
-    assert_eq!(map.vertex(1), Vertex2::from((0.0, 0.0)));
-    assert_eq!(map.vertex(2), Vertex2::from((1.0, 1.0)));
+    assert_eq!(map.vertex(1).unwrap(), Vertex2::from((0.0, 0.0)));
+    assert_eq!(map.vertex(2).unwrap(), Vertex2::from((1.0, 1.0)));
 }
 
 #[test]
@@ -207,7 +207,7 @@ fn one_sew_complete() {
     map.insert_vertex(2, (0.0, 1.0));
     map.insert_vertex(3, (0.0, 2.0));
     map.one_sew(1, 3);
-    assert_eq!(map.vertex(2), Vertex2::from((0.0, 1.5)));
+    assert_eq!(map.vertex(2).unwrap(), Vertex2::from((0.0, 1.5)));
 }
 
 #[test]
@@ -217,7 +217,7 @@ fn one_sew_incomplete_attributes() {
     map.insert_vertex(1, (0.0, 0.0));
     map.insert_vertex(2, (0.0, 1.0));
     map.one_sew(1, 3);
-    assert_eq!(map.vertex(2), Vertex2::from((0.0, 1.0)));
+    assert_eq!(map.vertex(2).unwrap(), Vertex2::from((0.0, 1.0)));
 }
 
 #[test]
@@ -226,7 +226,7 @@ fn one_sew_incomplete_beta() {
     map.insert_vertex(1, (0.0, 0.0));
     map.insert_vertex(2, (0.0, 1.0));
     map.one_sew(1, 2);
-    assert_eq!(map.vertex(2), Vertex2::from((0.0, 1.0)));
+    assert_eq!(map.vertex(2).unwrap(), Vertex2::from((0.0, 1.0)));
 }
 #[test]
 #[should_panic(expected = "No vertex defined on dart 2, use `one_link` instead of `one_sew`")]

honeycomb-core/src/spatial_repr/coords.rs

@@ -18,7 +18,7 @@ use std::ops::{
 // ------ CONTENT
 
 /// Coordinates-level error enum
-#[derive(Debug)]
+#[derive(Debug, PartialEq)]
 pub enum CoordsError {
     /// Error during the computation of the unit directional vector.
     ///

honeycomb-core/src/spatial_repr/vector.rs

@@ -29,7 +29,7 @@ use crate::{Coords2, CoordsError, CoordsFloat};
 /// let unit_y = Vector2::unit_y();
 ///
 /// assert_eq!(unit_x.dot(&unit_y), 0.0);
-/// assert_eq!(unit_x.normal_dir(), unit_y);
+/// assert_eq!(unit_x.normal_dir().unwrap(), unit_y);
 ///
 /// let two: FloatType = 2.0;
 /// let x_plus_y: Vector2<FloatType> = unit_x + unit_y;
@@ -127,14 +127,14 @@ impl<T: CoordsFloat> Vector2<T> {
     ///
     /// # Errors
     ///
-    /// This method will panic if called on a `Vector2` with a norm equal to zero, i.e. a null
-    /// `Vector2`.
+    /// This method will return an error if called on a `Vector2` with a norm equal to zero,
+    /// i.e. a null `Vector2`.
     ///
     /// # Example
     ///
     /// See [Vector2] example.
     ///
-    pub fn unit_dir(&self) -> Result<Vector2<T>, CoordsError> {
+    pub fn unit_dir(&self) -> Result<Self, CoordsError> {
         let norm = self.norm();
         if norm.is_zero() {
             Err(CoordsError::InvalidUnitDir)
@@ -150,25 +150,23 @@ impl<T: CoordsFloat> Vector2<T> {
     /// Return a [Vector2] indicating the direction of the normal to `self`. The norm of the
     /// returned struct is equal to one.
     ///
-    /// # Panics
+    /// # Errors
     ///
-    /// Because the returned `Vector2` is normalized, this method may panic under the same
-    /// condition as [`Self::unit_dir`].
+    /// This method will return an error if called on a `Vector2` with a norm equal to zero,
+    /// i.e. a null `Vector2`.
     ///
     /// # Example
     ///
     /// See [Vector2] example.
     ///
-    #[must_use = "constructed object is not used, consider removing this method call"]
-    pub fn normal_dir(&self) -> Self {
+    pub fn normal_dir(&self) -> Result<Vector2<T>, CoordsError> {
         Self {
             inner: Coords2 {
                 x: -self.inner.y,
                 y: self.inner.x,
             },
         }
         .unit_dir()
-        .unwrap()
     }
 
     /// Compute the dot product between two vectors
@@ -342,22 +340,28 @@ mod tests {
     fn normal_dir() {
         let along_x = Vector2::unit_x() * 4.0;
         let along_y = Vector2::unit_y() * 3.0;
-        assert!(almost_equal_vec(&along_x.normal_dir(), &Vector2::unit_y()));
         assert!(almost_equal_vec(
-            &Vector2::unit_x().normal_dir(),
+            &along_x.normal_dir().unwrap(),
+            &Vector2::unit_y()
+        ));
+        assert!(almost_equal_vec(
+            &Vector2::unit_x().normal_dir().unwrap(),
             &Vector2::unit_y()
         ));
-        assert!(almost_equal_vec(&along_y.normal_dir(), &-Vector2::unit_x()));
         assert!(almost_equal_vec(
-            &Vector2::unit_y().normal_dir(),
+            &along_y.normal_dir().unwrap(),
+            &-Vector2::unit_x()
+        ));
+        assert!(almost_equal_vec(
+            &Vector2::unit_y().normal_dir().unwrap(),
             &-Vector2::unit_x()
         ));
     }
 
     #[test]
-    #[should_panic(expected = "called `Result::unwrap()` on an `Err` value: InvalidUnitDir")]
     fn normal_dir_of_null_vel() {
         let origin: Vector2<FloatType> = Vector2::default();
-        let _ = origin.normal_dir(); // panics
+        let err = origin.normal_dir();
+        assert_eq!(err, Err(CoordsError::InvalidUnitDir));
     }
 }

honeycomb-examples/examples/render/squaremap_shift.rs

@@ -48,7 +48,9 @@ fn main() {
                 .map(|d_id| map.beta::<2>(d_id))
                 .collect();
             if !neighbors_vertex_cell.contains(&NULL_DART_ID) {
-                let current_value = map.vertex(*vertex_id);
+                // we can unwrap this safely since we built the grid manually
+                // & know that vertices are correctly defined
+                let current_value = map.vertex(*vertex_id).unwrap();
                 let _ = map.replace_vertex(
                     *vertex_id,
                     current_value + offsets[*vertex_id as usize % n_offsets],