georust / gdal

Rust bindings for GDAL
https://crates.io/crates/gdal
MIT License
363 stars 94 forks source link

Replace `OGRwkbGeometryType::Type` (a.k.a. c_uint) by `enum GeometryType` #252

Open ttencate opened 2 years ago

ttencate commented 2 years ago

Previously discussed on #250. The question was how extensible it should be, which I've briefly looked into.

  1. It looks like the last addition happened six years ago so it's definitely not changing frequently.
  2. The gdal_sys crate needs updating with every new release anyway due to the generated bindings, so adding new constants to the Rust enum manually should not be too much trouble. Or after the fact if nobody has a need for them yet.
  3. We can use #[non_exhaustive] so that client code is forced to reckon with new additions to the enum.
  4. What to do if an unsupported value is returned by a GDAL/OGR API? Assuming the Rust enum has been kept up to date, this can only mean that we are running with a newer version than we were compiled with. Is this even a supported scenario? I don't know if GDAL/OGR gives any ABI stability guarantees.
  5. What to do if a value of the Rust enum is not supported by the GDAL/OGR version we compiled with? Can we conditionally enable enum variants depending on the compile-time GDAL/OGR version?

Note that the issues are mostly hypothetical at the moment; OGRwkbGeometryType has the same values in all of the supported GDAL/OGR versions.

ChristianBeilschmidt commented 2 years ago

I'm in favor of wrapping these constants with enums.

lnicola commented 2 years ago

I honestly don't know what would be the best solution here, but I want to understand why you want this change.

ttencate commented 2 years ago

All of the above. Plus the ability to implement useful traits like Debug and Display on the type (which would have made #250 more idiomatic).

lnicola commented 2 years ago

What if we wrapped them in structs defined gdal? Something like:

#[derive(PartialEq, Eq)]
struct GeometryType(u32);

impl GeometryType {
    pub const Unknown: GeometryType = GeometryType(0);
    pub const Point: GeometryType = GeometryType(1);
    pub const LineString: GeometryType = GeometryType(2);
}
ttencate commented 2 years ago

In what way is that better than an enum though? If it's purely to allow unknown values through, we can also do that with an enum.

lnicola commented 2 years ago

You're right, it's not much better than an enum with an Unknown(u32) variant. I guess it would avoid some mapping code from the enum to integers and back.