Return a GeosResult on the various binary predicate methods

[mthh]

This PR wraps the result of the various binary predicates on GGeom and PreparedGGeom in a GeosResult in order to fix #10 .

I wanted to include a test case but I can't figure out how to trigger such an exception for now.

It bumps the version number to 1.1.0 as it breaks the API.

[antoine-de]

great 👍

you need to also change the examples in the readme to add a few ? (I tried to add skeptic to the project but I found it overcomplicated for the use case, so the examples are not built 😢 )

[antoine-de]

since the pattern can be found several times, maybe we can wrap this in a method:

fn check_geos_predicate<F>(val: i32, on_error: F) -> GeosResult<bool>
where
    F: FnOnce() -> Error {
    match val {
        1 => Ok(true),
        0 => Ok(false),
        _ => Err(on_error())
    } 
}

I'm not sure about the error function, but this way the error (and the string) is created only on error (clippy prefers Option::ok_or_else to Option::ok_or for this reason)

[mthh]

Sure I guess it will be really more clean. I'm not sure about the error function either, especially if we want to keep the name of the function that triggers the error.

[mthh]

I made a check_geos_predicate function which take the return value and an EnumVariant. This way I guess the error and the string are only created on error as you suggest (but it's not really elegant to match on this new big Enum in order to create the error), so don't hesitate to suggest me a better idea!

[antoine-de]

you could just have kept the error as before no ?

    pub fn is_ring(&self) -> GeosResult<bool> {
        let rv = unsafe { GEOSisRing(self.c_obj()) };
        check_geos_predicate(rv, || Error::GeosError("computing isRing".into()))
    }

but your solution is interesting as it removes the string.

It may be overkill, but maybe we could leverage the Failure crate for this ?

I pushed a toy implementation

honestly I'm not convinced that the complexity is worth it compared to the initial version with a lambda for the error. What do you think ?

I added some unit test that could be nice to get though.

[mthh]

Excepting this big PredicateType Enum, I find your solution pretty elegant, so I changed the code to follow it. Sorry for the lack of imagination of mine to tackle this issue.

This is a detail but as we are referring to 'libgeos method' in the error message, i don't think it really makes sense to transform the Enum member to snake case as they match GEOS function names in camel case (?)

[antoine-de]

ok, it's up to you, both are fine for me

[antoine-de]

great work! 👍

[mthh]

@antoine-de Sorry, I planned to publish this new release on crates.io but I do not seem to have the necessary rights. Any advice ?

[antoine-de]

I just added the all of the GeoRust/core team to the crates.io owner, should work now, sorry 😕

However I published the crates.io yesterday before reading the crates.io documentation, so we need to wait for another PR to test if you can publish a version.

btw not related at all, but we'll go to the state of the map france this year (to explain why we needed rust-geos 😜 ), will you go there to ?

[mthh]

Oh great for SOTM-fr, I will be there too! (only from Friday night I think, because of other commitments + I didn't submit anything this year). For sure I will not miss your intervention if it is Saturday (otherwise let's just grab a coffee there!)