refactor: return None for out-of-range row indexes, part III.

* For a table type X, X::row now returns Option instead of Result BREAKING CHANGE: return values changes
tskit-dev · Oct 31, 2022 · bf6fa5b · bf6fa5b
1 parent cb580f1
commit bf6fa5b
Show file tree

Hide file tree

Showing 11 changed files with 69 additions and 99 deletions.
diff --git a/src/_macros.rs b/src/_macros.rs
@@ -186,7 +186,7 @@ macro_rules! err_if_not_tracking_samples {
 // as it implies $row is out of range.
 macro_rules! table_row_access {
     ($row: expr, $table: expr, $row_fn: ident) => {
-        $row_fn($table, $row).ok_or_else(|| TskitError::IndexError {})
+        $row_fn($table, $row)
     };
 }
 

diff --git a/src/edge_table.rs b/src/edge_table.rs
@@ -143,15 +143,12 @@ impl<'a> EdgeTable<'a> {
     ///
     /// * `r`: the row id.
     ///
-    /// # Errors
-    ///
-    /// [`TskitError::IndexError`] if `r` is out of range.
-    pub fn row<E: Into<EdgeId> + Copy>(&self, r: E) -> Result<EdgeTableRow, TskitError> {
-        let ri = r.into();
-        if ri < 0 {
-            return Err(crate::TskitError::IndexError);
-        }
-        table_row_access!(ri.0, self, make_edge_table_row)
+    /// # Returns
+    ///
+    /// * `Some(row)` if `r` is valid
+    /// * `None` otherwise
+    pub fn row<E: Into<EdgeId> + Copy>(&self, r: E) -> Option<EdgeTableRow> {
+        table_row_access!(r.into().0, self, make_edge_table_row)
     }
 }
 

diff --git a/src/individual_table.rs b/src/individual_table.rs
@@ -247,18 +247,13 @@ impl<'a> IndividualTable<'a> {
     ///
     /// * `r`: the row id.
     ///
-    /// # Errors
+    /// # Returns
     ///
-    /// [`TskitError::IndexError`] if `r` is out of range.
-    pub fn row<I: Into<IndividualId> + Copy>(
-        &self,
-        r: I,
-    ) -> Result<IndividualTableRow, TskitError> {
-        let ri = r.into();
-        if ri < 0 {
-            return Err(crate::TskitError::IndexError);
-        }
-        table_row_access!(ri.0, self, make_individual_table_row)
+    /// * `Some(row)` if `r` is valid
+    /// * `None` otherwise
+    pub fn row<I: Into<IndividualId> + Copy>(&self, r: I) -> Option<IndividualTableRow> {
+        let ri = r.into().0;
+        table_row_access!(ri, self, make_individual_table_row)
     }
 }
 

diff --git a/src/migration_table.rs b/src/migration_table.rs
@@ -190,15 +190,13 @@ impl<'a> MigrationTable<'a> {
     ///
     /// * `r`: the row id.
     ///
-    /// # Errors
+    /// # Returns
     ///
-    /// [`TskitError::IndexError`] if `r` is out of range.
-    pub fn row<M: Into<MigrationId> + Copy>(&self, r: M) -> Result<MigrationTableRow, TskitError> {
-        let ri = r.into();
-        if ri < 0 {
-            return Err(crate::TskitError::IndexError);
-        }
-        table_row_access!(r.into().0, self, make_migration_table_row)
+    /// * `Some(row)` if `r` is valid
+    /// * `None` otherwise
+    pub fn row<M: Into<MigrationId> + Copy>(&self, r: M) -> Option<MigrationTableRow> {
+        let ri = r.into().0;
+        table_row_access!(ri, self, make_migration_table_row)
     }
 }
 

diff --git a/src/mutation_table.rs b/src/mutation_table.rs
@@ -180,15 +180,13 @@ impl<'a> MutationTable<'a> {
     ///
     /// * `r`: the row id.
     ///
-    /// # Errors
+    /// # Returns
     ///
-    /// [`TskitError::IndexError`] if `r` is out of range.
-    pub fn row<M: Into<MutationId> + Copy>(&self, r: M) -> Result<MutationTableRow, TskitError> {
-        let ri = r.into();
-        if ri < 0 {
-            return Err(crate::TskitError::IndexError);
-        }
-        table_row_access!(ri.0, self, make_mutation_table_row)
+    /// * `Some(row)` if `r` is valid
+    /// * `None` otherwise
+    pub fn row<M: Into<MutationId> + Copy>(&self, r: M) -> Option<MutationTableRow> {
+        let ri = r.into().0;
+        table_row_access!(ri, self, make_mutation_table_row)
     }
 }
 

diff --git a/src/node_table.rs b/src/node_table.rs
@@ -197,15 +197,13 @@ impl<'a> NodeTable<'a> {
     ///
     /// * `r`: the row id.
     ///
-    /// # Errors
+    /// # Returns
     ///
-    /// [`TskitError::IndexError`] if `r` is out of range.
-    pub fn row<N: Into<NodeId> + Copy>(&self, r: N) -> Result<NodeTableRow, TskitError> {
-        let ri = r.into();
-        if ri < 0 {
-            return Err(crate::TskitError::IndexError);
-        }
-        table_row_access!(ri.0, self, make_node_table_row)
+    /// * `Some(row)` if `r` is valid
+    /// * `None` otherwise
+    pub fn row<N: Into<NodeId> + Copy>(&self, r: N) -> Option<NodeTableRow> {
+        let ri = r.into().0;
+        table_row_access!(ri, self, make_node_table_row)
     }
 
     /// Obtain a vector containing the indexes ("ids")

diff --git a/src/population_table.rs b/src/population_table.rs
@@ -101,18 +101,13 @@ impl<'a> PopulationTable<'a> {
     ///
     /// * `r`: the row id.
     ///
-    /// # Errors
+    /// # Returns
     ///
-    /// [`TskitError::IndexError`] if `r` is out of range.
-    pub fn row<P: Into<PopulationId> + Copy>(
-        &self,
-        r: P,
-    ) -> Result<PopulationTableRow, TskitError> {
-        let ri = r.into();
-        if ri < 0 {
-            return Err(crate::TskitError::IndexError);
-        }
-        table_row_access!(ri.0, self, make_population_table_row)
+    /// * `Some(row)` if `r` is valid
+    /// * `None` otherwise
+    pub fn row<P: Into<PopulationId> + Copy>(&self, r: P) -> Option<PopulationTableRow> {
+        let ri = r.into().0;
+        table_row_access!(ri, self, make_population_table_row)
     }
 }
 

diff --git a/src/provenance.rs b/src/provenance.rs
@@ -146,21 +146,12 @@ impl<'a> ProvenanceTable<'a> {
 
     /// Obtain a [`ProvenanceTableRow`] for row `row`.
     ///
-    /// # Errors
+    /// # Returns
     ///
-    /// [`TskitError::IndexError`] if `r` is out of range.
-    pub fn row<P: Into<ProvenanceId> + Copy>(
-        &'a self,
-        row: P,
-    ) -> Result<ProvenanceTableRow, TskitError> {
-        if row.into() < 0 {
-            Err(TskitError::IndexError)
-        } else {
-            match make_provenance_row(self, row.into().0) {
-                Some(x) => Ok(x),
-                None => Err(TskitError::IndexError),
-            }
-        }
+    /// * `Some(row)` if `r` is valid
+    /// * `None` otherwise
+    pub fn row<P: Into<ProvenanceId> + Copy>(&'a self, row: P) -> Option<ProvenanceTableRow> {
+        make_provenance_row(self, row.into().0)
     }
 
     /// Return an iterator over rows of the table.

diff --git a/src/site_table.rs b/src/site_table.rs
@@ -131,15 +131,13 @@ impl<'a> SiteTable<'a> {
     ///
     /// * `r`: the row id.
     ///
-    /// # Errors
-    ///
-    /// [`TskitError::IndexError`] if `r` is out of range.
-    pub fn row<S: Into<SiteId> + Copy>(&self, r: S) -> Result<SiteTableRow, TskitError> {
-        let ri = r.into();
-        if ri < 0 {
-            return Err(crate::TskitError::IndexError);
-        }
-        table_row_access!(r.into().0, self, make_site_table_row)
+    /// # Returns
+    ///
+    /// * `Some(row)` if `r` is valid
+    /// * `None` otherwise
+    pub fn row<S: Into<SiteId> + Copy>(&self, r: S) -> Option<SiteTableRow> {
+        let ri = r.into().0;
+        table_row_access!(ri, self, make_site_table_row)
     }
 }
 

diff --git a/src/table_collection.rs b/src/table_collection.rs
@@ -1670,11 +1670,11 @@ mod test {
             .unwrap();
 
         match tables.nodes().row(NodeId::from(0)) {
-            Ok(x) => match x.population {
+            Some(x) => match x.population {
                 PopulationId(0) => (),
                 _ => panic!("expected PopulationId(0)"),
             },
-            Err(_) => panic!("expected Ok(_)"),
+            None => panic!("expected Some(_)"),
         };
     }
 
@@ -1847,7 +1847,7 @@ mod test_adding_node {
         assert!(tables
             .populations()
             .row(tables.nodes().population(row_id).unwrap())
-            .is_err());
+            .is_none());
 
         let row_id = tables
             .add_node(0, 0.0, PopulationId::NULL, IndividualId::from(17))
@@ -1862,7 +1862,7 @@ mod test_adding_node {
         assert!(tables
             .individuals()
             .row(tables.nodes().individual(row_id).unwrap())
-            .is_err());
+            .is_none());
     }
 
     #[test]

diff --git a/tests/empty_table_collection.rs b/tests/empty_table_collection.rs
@@ -4,24 +4,24 @@ fn test_empty_table_collection() {
 
     let tables = tskit::TableCollection::new(10.).unwrap();
 
-    assert!(tables.edges().row(-1).is_err());
-    assert!(tables.edges().row(0).is_err());
-    assert!(tables.nodes().row(-1).is_err());
-    assert!(tables.nodes().row(0).is_err());
-    assert!(tables.sites().row(-1).is_err());
-    assert!(tables.sites().row(0).is_err());
-    assert!(tables.mutations().row(-1).is_err());
-    assert!(tables.mutations().row(0).is_err());
-    assert!(tables.individuals().row(-1).is_err());
-    assert!(tables.individuals().row(0).is_err());
-    assert!(tables.populations().row(-1).is_err());
-    assert!(tables.populations().row(0).is_err());
-    assert!(tables.migrations().row(-1).is_err());
-    assert!(tables.migrations().row(0).is_err());
+    assert!(tables.edges().row(-1).is_none());
+    assert!(tables.edges().row(0).is_none());
+    assert!(tables.nodes().row(-1).is_none());
+    assert!(tables.nodes().row(0).is_none());
+    assert!(tables.sites().row(-1).is_none());
+    assert!(tables.sites().row(0).is_none());
+    assert!(tables.mutations().row(-1).is_none());
+    assert!(tables.mutations().row(0).is_none());
+    assert!(tables.individuals().row(-1).is_none());
+    assert!(tables.individuals().row(0).is_none());
+    assert!(tables.populations().row(-1).is_none());
+    assert!(tables.populations().row(0).is_none());
+    assert!(tables.migrations().row(-1).is_none());
+    assert!(tables.migrations().row(0).is_none());
 
     #[cfg(feature = "provenance")]
     {
-        assert!(tables.provenances().row(-1).is_err());
-        assert!(tables.provenances().row(0).is_err());
+        assert!(tables.provenances().row(-1).is_none());
+        assert!(tables.provenances().row(0).is_none());
     }
 }