Implements `Geometry::make_valid`. #356

metasim · 2022-12-31T17:39:06Z

I agree to follow the project's code of conduct.
I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Partially addresses #306.

cc: @thomas001

src/metadata.rs

src/vector/geometry.rs

CHANGES.md

src/vector/geometry.rs

…try`.

src/vector/vector_tests/mod.rs

lnicola · 2023-01-10T18:05:15Z

src/cpl.rs

+    fn from(pairs: &[(&str, &str); N]) -> Self {
+        let mut result = Self::default();
+        for (k, v) in pairs {
+            result.set_name_value(k, v).expect("valid key/value pair");


Suggested change

result.set_name_value(k, v).expect("valid key/value pair");

result.set_name_value(k, v).expect("invalid key/value pair");

I think the message would be something like valid key/value pair: "Invalid characters in name: '#'", which doesn't make sense.

And going back a little, I'm not sure what to say about this impl. from should be infallible, so try_from would be better, which this clearly isn't. On the other hand, it's probably going to be used with compile-constant keys and values. Is that why you've implemented it for fixed-size arrays (as opposed to &[(&str, &str)])?

Is that why you've implemented it for fixed-size arrays

No. More of a result of me playing type golf; Without the const parameter I get this error. Probably me being dumb.

The fact that set_name_value is fallible is really annoying, but understandable. Going with try_from pretty much eliminates the benefits of having this. So if you're in doubt about it, I think I should just remove it all together.

Oof, yes, I think you need to cast it, like in https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0c8e96e6b31a1d7ac28a76cce7abbd1b. Anyway, I suppose it's fine.

I think the message would be something like valid key/value pair: "Invalid characters in name: '#'", which doesn't make sense.

Are you 100% sure? (I'm not) 😄

Per the docs

We recommend that expect messages are used to describe the reason you expect the Result should be Ok.

Can you test? 😀 I can't right now, sorry.

My reading of the docs is something like CslStringList::try_from(&["FOO", "BAR"]).expect("obviously correct"), that is, user code. It's different in library code which can and will actually panic.

I'm not sure I'm following your line of thinking here. Happy to submit a new PR revisiting this.

Indeed, this produces:

thread 'cpl::tests::from_impl' panicked at 'valid key/value pair: BadArgument("Invalid characters in name: 'TWO#'")', src/cpl.rs:204:41

compared to a random example from libcore:

self.checked_add(rhs).expect("overflow when adding durations")

src/vector/geometry.rs

lnicola · 2023-01-10T18:23:55Z

src/vector/geometry.rs


    #[test]
    #[allow(clippy::float_cmp)]
    pub fn test_area() {
+        let _nolog = SuppressGDALErrorLog::new();


I'm not sure we should hide these GDAL warnings, especially given that the tests are run on multiple threads. GDAL doesn't synchronize accesses to that list (not that it makes sense to use it from multiple threads), so there's a chance we'll corrupt its handler stack.

I only put these in in tests that are supposed to generate errors. e.g.

layer.set_attribute_filter("foo = bar").unwrap_err(),

According to the API docs the approach I used should be thread safe.

This pushes a new error handler on the thread-local error handler stack. This handler will be used until removed with CPLPopErrorHandler().

My reasoning behind this utility was to make life easier when going through test logs when there is a failure. We have a bunch of stay messages that look like something went wrong when the error condition was expected. I really don't like logs that have false error messages in them. IMO, in good Unix style, successes should be quiet.

Ah, I missed it being thread-local. Should be fine, then 👍.

lnicola · 2023-01-10T18:28:38Z

src/vector/layer.rs

@@ -251,7 +251,8 @@ pub trait LayerAccess: Sized {
    }

    /// Set a feature on this layer layer.
-    /// Refer[SetFeature](https://gdal.org/doxygen/classOGRLayer.html#a681139bfd585b74d7218e51a32144283)
+    ///
+    /// Refer [SetFeature](https://gdal.org/doxygen/classOGRLayer.html#a681139bfd585b74d7218e51a32144283)


Consistency nit: below it says Refer: , maybe See would sound better.

I don't like Refer either... but was trying to be consistent (but forgot the colon here). I'll update.

lnicola

The error handling changes seem like a really bad idea.

I'm also unsure about the From implementation for CslStringList, but that's probably less important. From (sic!) the docs:

Note: This trait must not fail. The From trait is intended for perfect conversions. If the conversion can fail or is not perfect, use TryFrom.

Other than that, LGTM.

Co-authored-by: Laurențiu Nicola <[email protected]>

metasim · 2023-01-10T19:27:27Z

The error handling changes seem like a really bad idea.

I detailed my rationale here. Only doing it because a) I believe successful tests should not report error messages, and b) according to the gdal docs, the implementation should be thread-safe.

metasim · 2023-01-10T19:29:39Z

I'm also unsure about the From implementation for CslStringList, but that's probably less important.

I'm also unsure about this. Maybe I should just drop this impl until we figure out a more ergonomic way to construct static CslStringLists?

metasim · 2023-01-10T19:37:00Z

src/vector/geometry.rs


    #[test]
    #[allow(clippy::float_cmp)]
    pub fn test_area() {
+        let _nolog = SuppressGDALErrorLog::new();


Without this specific suppression, the log message is

Warning 1: OGR_G_Area() called against non-surface geometry type.

lnicola · 2023-01-10T19:40:32Z

We could also implement TryFrom, which seems like a good thing to have anyway.

bors d+

bors · 2023-01-10T19:40:34Z

✌️ metasim can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

metasim · 2023-01-10T19:47:00Z

bors r+

metasim · 2023-01-10T19:47:53Z

@lnicola Thanks for slogging through this with me! I'm really happy with the result. Wouldn't be half as good without your feedback! 😃

bors · 2023-01-10T19:50:39Z

Build succeeded: