-
Notifications
You must be signed in to change notification settings - Fork 94
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
Add support for Integer64 #80
Conversation
@@ -68,6 +71,11 @@ impl<'a> Feature<'a> { | |||
let rv = unsafe { gdal_sys::OGR_F_GetFieldAsInteger(self.c_feature, field_id) }; | |||
Ok(FieldValue::IntegerValue(rv as i32)) | |||
}, | |||
#[cfg(feature = "gdal_2_2")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need 2.0, but this is a mess anyway :-(.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess we have to check if the feature is >= gdal_2_x. Maybe we could do a not "gdal_1_11"? Could you look into this?
My suggestion for the future is to rebuild the crate into feature gated modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could do a not "gdal_1_11"? Could you look into this?
That might end up as any(feature = "gdal_2_2", feature = "gdal_2_4", feature = "gdal_3_0")
.
My suggestion for the future is to rebuild the crate into feature gated modules.
Sigh. I'm not sure this works too well -- it's an enum variant, after all.
Maybe we could always compile it in, but fail (or panic at runtime) if the feature wasn't enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #86, I faced a similar issue and made a "superset" feature for the major version, maybe it would make sense to do something similar here?
gdal_2_2 = ["gdal_2", "gdal-sys/min_gdal_version_2_2"]
gdal_2_4 = ["gdal_2", "gdal-sys/min_gdal_version_2_4"]
gdal_2 = []
#[cfg(feature = "gdal_2")]
...
4cd2e7b
to
a487a27
Compare
I wonder if it's better to drop support for older versions. It's easier to get an up-to-date GDAL these days. |
Would you port this to the feature_gate_versions branch? I think it solves the issues with different versions and i like to merge it into master soon. |
@jdroenner I'll port it if it gets merged, but it feels like a lot of complexity TBH. Did you pick a solution for the enum versioning? |
With the change we only support gdal 2 and 3. So i guess the enum can stay in common for now. My idea was to split enums also by version if needed. So we can have different enums for different versions. |
Fixes #79