gdal
gdal copied to clipboard
Replace `OGRwkbGeometryType::Type` (a.k.a. c_uint) by `enum GeometryType`
Previously discussed on #250. The question was how extensible it should be, which I've briefly looked into.
- It looks like the last addition happened six years ago so it's definitely not changing frequently.
- 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. - We can use
#[non_exhaustive]
so that client code is forced to reckon with new additions to the enum. - 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.
- 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.
I'm in favor of wrapping these constants with enums.
-
#[non_exhaustive]
isn't worse than fiddling with these numbers.- We could use
wkbUnknown
as long as we don't know it.
- We could use
- You are right that new version-bindings could only lead to new values. However, we would need some mechanism to catch new versions. Last time, it took us some while for new bindings. Moreover, can we have a mechanism that checks for all values being covered?
- I guess we can conditionally enable variants. In the worst case, we can conditionally switch between enums.
I honestly don't know what would be the best solution here, but I want to understand why you want this change.
- is it because it's annoying to import
gdal-sys
and type thoseOGRwkb
prefixes? - is it so matching on the geometry gets a little nicer?
- or because an enum would be more strongly-typed, so you can't pass a wrong value by mistake?
- or maybe something else?
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).
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);
}
- you don't have to import
gdal-sys
and it has a nicer name - you can use match (and must exhaustively do so)
- it's strongly-typed
- you can implement
Debug
andDisplay
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
.
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.