Merge #362

362: Replace `CslStringList` `From` implementation with `TryFrom` r=metasim a=lnicola

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

This isn't _that_ ugly, and I think it's better than panicking in a `From` implementation.

Co-authored-by: Laurențiu Nicola <lnicola@dend.ro>

CHANGES.md

@@ -6,7 +6,11 @@
 
   - <https://github.com/georust/gdal/pull/363>
 
-- Added c_rasterband method to Rasterband to obtain the raw C pointer to GDALRasterBandH
+- Added a `TryFrom` array implementation for `CslStringList`
+
+  - <https://github.com/georust/gdal/pull/362>
+
+- Added `Rasterband::c_rasterband` to obtain the raw C pointer to `GDALRasterBandH`
 
   - <https://github.com/georust/gdal/pull/359>
 

src/cpl.rs

@@ -164,44 +164,25 @@ impl Debug for CslStringList {
     }
 }
 
-/// Convenience shorthand for specifying an empty `CslStringList` to functions accepting
-/// `Into<CslStringList>`.
-///
-/// # Example
-///
-/// ```rust, no_run
-/// use gdal::cpl::CslStringList;
-/// fn count_opts<O: Into<CslStringList>>(opts: O) -> usize {
-///     opts.into().len()
-/// }
-///
-/// assert_eq!(count_opts(()), 0);
-/// ```
-impl From<()> for CslStringList {
-    fn from(_: ()) -> Self {
-        CslStringList::default()
-    }
-}
-
 /// Convenience for creating a [`CslStringList`] from a slice of _key_/_value_ tuples.
 ///
 /// # Example
 ///
 /// ```rust, no_run
 /// use gdal::cpl::CslStringList;
-/// fn count_opts<O: Into<CslStringList>>(opts: O) -> usize {
-///     opts.into().len()
-/// }
 ///
-/// assert_eq!(count_opts(&[("One", "1"), ("Two", "2"), ("Three", "3")]), 3);
+/// let opts = CslStringList::try_from(&[("One", "1"), ("Two", "2"), ("Three", "3")]).expect("known valid");
+/// assert_eq!(opts.len(), 3);
 /// ```
-impl<const N: usize> From<&[(&str, &str); N]> for CslStringList {
-    fn from(pairs: &[(&str, &str); N]) -> Self {
+impl<const N: usize> TryFrom<&[(&str, &str); N]> for CslStringList {
+    type Error = GdalError;
+
+    fn try_from(pairs: &[(&str, &str); N]) -> Result<Self> {
         let mut result = Self::default();
         for (k, v) in pairs {
-            result.set_name_value(k, v).expect("valid key/value pair");
+            result.set_name_value(k, v)?;
         }
-        result
+        Ok(result)
     }
 }
 
@@ -269,6 +250,15 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn try_from_impl() -> Result<()> {
+        let l = CslStringList::try_from(&[("ONE", "1"), ("TWO", "2")])?;
+        assert!(matches!(l.fetch_name_value("ONE"), Ok(Some(s)) if s == *"1"));
+        assert!(matches!(l.fetch_name_value("TWO"), Ok(Some(s)) if s == *"2"));
+
+        Ok(())
+    }
+
     #[test]
     fn debug_fmt() -> Result<()> {
         let l = fixture()?;

src/vector/geometry.rs

@@ -475,38 +475,34 @@ impl Geometry {
     ///
     /// # Example
     /// ```rust, no_run
+    /// use gdal::cpl::CslStringList;
     /// use gdal::vector::Geometry;
     /// # fn main() -> gdal::errors::Result<()> {
     /// let src = Geometry::from_wkt("POLYGON ((0 0, 10 10, 0 10, 10 0, 0 0))")?;
-    /// let dst = src.make_valid(())?;
+    /// let dst = src.make_valid(&CslStringList::new())?;
     /// assert_eq!("MULTIPOLYGON (((10 0, 0 0, 5 5, 10 0)),((10 10, 5 5, 0 10, 10 10)))", dst.wkt()?);
     /// # Ok(())
     /// # }
     /// ```
-    pub fn make_valid<O: Into<CslStringList>>(&self, opts: O) -> Result<Geometry> {
-        let opts = opts.into();
-
-        fn inner(geom: &Geometry, opts: CslStringList) -> Result<Geometry> {
-            #[cfg(all(major_ge_3, minor_ge_4))]
-            let c_geom = unsafe { gdal_sys::OGR_G_MakeValidEx(geom.c_geometry(), opts.as_ptr()) };
-
-            #[cfg(not(all(major_ge_3, minor_ge_4)))]
-            let c_geom = {
-                if !opts.is_empty() {
-                    return Err(GdalError::BadArgument(
-                        "Options to make_valid require GDAL >= 3.4".into(),
-                    ));
-                }
-                unsafe { gdal_sys::OGR_G_MakeValid(geom.c_geometry()) }
-            };
-
-            if c_geom.is_null() {
-                Err(_last_null_pointer_err("OGR_G_MakeValid"))
-            } else {
-                Ok(unsafe { Geometry::with_c_geometry(c_geom, true) })
+    pub fn make_valid(&self, opts: &CslStringList) -> Result<Geometry> {
+        #[cfg(all(major_ge_3, minor_ge_4))]
+        let c_geom = unsafe { gdal_sys::OGR_G_MakeValidEx(self.c_geometry(), opts.as_ptr()) };
+
+        #[cfg(not(all(major_ge_3, minor_ge_4)))]
+        let c_geom = {
+            if !opts.is_empty() {
+                return Err(GdalError::BadArgument(
+                    "Options to make_valid require GDAL >= 3.4".into(),
+                ));
             }
+            unsafe { gdal_sys::OGR_G_MakeValid(self.c_geometry()) }
+        };
+
+        if c_geom.is_null() {
+            Err(_last_null_pointer_err("OGR_G_MakeValid"))
+        } else {
+            Ok(unsafe { Geometry::with_c_geometry(c_geom, true) })
         }
-        inner(self, opts)
     }
 }
 
@@ -691,7 +687,7 @@ mod tests {
     /// Simple clone case.
     pub fn test_make_valid_clone() {
         let src = Geometry::from_wkt("POINT (0 0)").unwrap();
-        let dst = src.make_valid(());
+        let dst = src.make_valid(&CslStringList::new());
         assert!(dst.is_ok());
     }
 
@@ -700,15 +696,15 @@ mod tests {
     pub fn test_make_valid_invalid() {
         let _nolog = SuppressGDALErrorLog::new();
         let src = Geometry::from_wkt("LINESTRING (0 0)").unwrap();
-        let dst = src.make_valid(());
+        let dst = src.make_valid(&CslStringList::new());
         assert!(dst.is_err());
     }
 
     #[test]
     /// Repairable case (self-intersecting)
     pub fn test_make_valid_repairable() {
         let src = Geometry::from_wkt("POLYGON ((0 0, 10 10, 0 10, 10 0, 0 0))").unwrap();
-        let dst = src.make_valid(());
+        let dst = src.make_valid(&CslStringList::new());
         assert!(dst.is_ok());
     }
 
@@ -719,7 +715,8 @@ mod tests {
         let src =
             Geometry::from_wkt("POLYGON ((0 0, 0 10, 10 10, 10 0, 0 0),(5 5, 15 10, 15 0, 5 5))")
                 .unwrap();
-        let dst = src.make_valid(&[("STRUCTURE", "LINEWORK")]);
+        let opts = CslStringList::try_from(&[("STRUCTURE", "LINEWORK")]).unwrap();
+        let dst = src.make_valid(&opts);
         assert!(dst.is_ok(), "{dst:?}");
     }
 }