refactor: provenance table getters now return Option (#369)

* empty provenance records are now errors

BREAKING CHANGE: return types and behavior for empty
data.

Author: molpopgen

src/_macros.rs

@@ -104,11 +104,11 @@ macro_rules! unsafe_tsk_ragged_column_access {
 #[allow(unused_macros)]
 macro_rules! unsafe_tsk_ragged_char_column_access {
     ($i: expr, $lo: expr, $hi: expr, $owner: expr, $array: ident, $offset_array: ident, $offset_array_len: ident) => {{
-        let i = $crate::SizeType::try_from($i)?;
+        let i = $crate::SizeType::try_from($i).ok()?;
         if $i < $lo || i >= $hi {
-            Err(TskitError::IndexError {})
+            None
         } else if $owner.$offset_array_len == 0 {
-            Ok(None)
+            None
         } else {
             assert!(!$owner.$array.is_null());
             assert!(!$owner.$offset_array.is_null());
@@ -119,13 +119,13 @@ macro_rules! unsafe_tsk_ragged_char_column_access {
                 $owner.$offset_array_len as tsk_size_t
             };
             if start == stop {
-                Ok(None)
+                None
             } else {
                 let mut buffer = String::new();
                 for i in start..stop {
                     buffer.push(unsafe { *$owner.$array.offset(i as isize) as u8 as char });
                 }
-                Ok(Some(buffer))
+                Some(buffer)
             }
         }
     }};
@@ -1028,6 +1028,9 @@ macro_rules! provenance_table_add_row {
     ($(#[$attr:meta])* => $name: ident, $self: ident, $table: expr) => {
         $(#[$attr])*
         pub fn $name(&mut $self, record: &str) -> Result<$crate::ProvenanceId, $crate::TskitError> {
+            if record.is_empty() {
+                return Err($crate::TskitError::ValueError{got: "empty string".to_string(), expected: "provenance record".to_string()})
+            }
             let timestamp = humantime::format_rfc3339(std::time::SystemTime::now()).to_string();
             let rv = unsafe {
                 $crate::bindings::tsk_provenance_table_add_row(

src/provenance.rs

@@ -14,7 +14,7 @@
 
 use crate::bindings as ll_bindings;
 use crate::SizeType;
-use crate::{tsk_id_t, tsk_size_t, ProvenanceId, TskitError};
+use crate::{tsk_id_t, tsk_size_t, ProvenanceId};
 use ll_bindings::{tsk_provenance_table_free, tsk_provenance_table_init};
 
 #[derive(Eq)]
@@ -47,8 +47,8 @@ impl std::fmt::Display for ProvenanceTableRow {
 fn make_provenance_row(table: &ProvenanceTable, pos: tsk_id_t) -> Option<ProvenanceTableRow> {
     Some(ProvenanceTableRow {
         id: pos.into(),
-        timestamp: table.timestamp(pos).ok()?,
-        record: table.record(pos).ok()?,
+        timestamp: table.timestamp(pos)?,
+        record: table.record(pos)?,
     })
 }
 
@@ -103,47 +103,66 @@ impl<'a> ProvenanceTable<'a> {
 
     /// Get the ISO-formatted time stamp for row `row`.
     ///
-    /// # Errors
+    /// # Returns
+    ///
+    /// * `Some(String)` if `row` is valid.
+    /// * `None` otherwise.
     ///
-    /// [`TskitError::IndexError`] if `r` is out of range.
-    pub fn timestamp<P: Into<ProvenanceId> + Copy>(&'a self, row: P) -> Result<String, TskitError> {
-        match unsafe_tsk_ragged_char_column_access!(
+    /// # Examples
+    ///
+    /// ```
+    /// use tskit::TableAccess;
+    /// let mut tables = tskit::TableCollection::new(10.).unwrap();
+    /// assert!(tables.add_provenance("foo").is_ok());
+    /// if let Some(timestamp) = tables.provenances().timestamp(0) {
+    ///  // then 0 is a valid row in the provenance table
+    /// }
+    /// # else {
+    /// # panic!("Expected Some(timestamp)");
+    /// # }
+    /// ```
+    pub fn timestamp<P: Into<ProvenanceId> + Copy>(&'a self, row: P) -> Option<String> {
+        unsafe_tsk_ragged_char_column_access!(
             row.into().0,
             0,
             self.num_rows(),
             self.table_,
             timestamp,
             timestamp_offset,
             timestamp_length
-        ) {
-            Ok(Some(string)) => Ok(string),
-            Ok(None) => Err(crate::TskitError::ValueError {
-                got: String::from("None"),
-                expected: String::from("String"),
-            }),
-            Err(e) => Err(e),
-        }
+        )
     }
 
     /// Get the provenance record for row `row`.
     ///
-    /// # Errors
+    /// # Returns
+    ///
+    /// * `Some(String)` if `row` is valid.
+    /// * `None` otherwise.
+    ///
+    /// # Examples
     ///
-    /// [`TskitError::IndexError`] if `r` is out of range.
-    pub fn record<P: Into<ProvenanceId> + Copy>(&'a self, row: P) -> Result<String, TskitError> {
-        match unsafe_tsk_ragged_char_column_access!(
+    /// ```
+    /// use tskit::TableAccess;
+    /// let mut tables = tskit::TableCollection::new(10.).unwrap();
+    /// assert!(tables.add_provenance("foo").is_ok());
+    /// if let Some(record) = tables.provenances().record(0) {
+    ///  // then 0 is a valid row in the provenance table
+    ///  # assert_eq!(record, "foo");
+    /// }
+    /// # else {
+    /// # panic!("Expected Some(timestamp)");
+    /// # }
+    pub fn record<P: Into<ProvenanceId> + Copy>(&'a self, row: P) -> Option<String> {
+        unsafe_tsk_ragged_char_column_access!(
             row.into().0,
             0,
             self.num_rows(),
             self.table_,
             record,
             record_offset,
             record_length
-        ) {
-            Ok(Some(string)) => Ok(string),
-            Ok(None) => Ok(String::from("")),
-            Err(e) => Err(e),
-        }
+        )
     }
 
     /// Obtain a [`ProvenanceTableRow`] for row `row`.
@@ -201,16 +220,14 @@ mod test_provenances {
         // check for tables...
         let mut tables = crate::TableCollection::new(10.).unwrap();
         let s = String::from("");
-        let row_id = tables.add_provenance(&s).unwrap();
-        let _ = tables.provenances().row(row_id).unwrap();
+        assert!(tables.add_provenance(&s).is_err());
 
         // and for tree sequences...
         tables.build_index().unwrap();
         let mut ts = tables
             .tree_sequence(crate::TreeSequenceFlags::default())
             .unwrap();
-        let row_id = ts.add_provenance(&s).unwrap();
-        let _ = ts.provenances().row(row_id).unwrap();
+        assert!(ts.add_provenance(&s).is_err())
     }
 
     #[test]

src/trees.rs

@@ -502,6 +502,12 @@ impl TreeSequence {
     /// # }
     /// ```
     pub fn add_provenance(&mut self, record: &str) -> Result<crate::ProvenanceId, TskitError> {
+        if record.is_empty() {
+            return Err(TskitError::ValueError {
+                got: "empty string".to_string(),
+                expected: "provenance record".to_string(),
+            });
+        }
         let timestamp = humantime::format_rfc3339(std::time::SystemTime::now()).to_string();
         let rv = unsafe {
             ll_bindings::tsk_provenance_table_add_row(