Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ttencate opened this issue Feb 1, 2022 · 6 comments
Open

Comments

@ttencate
Copy link
Contributor

ttencate commented Feb 1, 2022

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
Copy link
Contributor

ChristianBeilschmidt commented Feb 2, 2022

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.
  • 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.

@lnicola
Copy link
Member

lnicola commented Feb 2, 2022

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 those OGRwkb 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?

@ttencate
Copy link
Contributor Author

ttencate commented Feb 2, 2022

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
Copy link
Member

lnicola commented Feb 2, 2022

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 and Display

@ttencate
Copy link
Contributor Author

ttencate commented Feb 2, 2022

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
Copy link
Member

lnicola commented Feb 2, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants