georust / geozero

Zero-Copy reading and writing of geospatial data.
Apache License 2.0
321 stars 30 forks source link

update flatgeobuf #179

Closed michaelkirk closed 7 months ago

michaelkirk commented 7 months ago

Flatgeobuf release notes 4.0.0

https://github.com/flatgeobuf/flatgeobuf/blob/master/src/rust/CHANGELOG.md#400---2023-10-14

[4.0.0] - 2023-10-14

Breaking: select_all and select_bbox now return a FeatureIter instead of a modified Self type. Ensure reading from tls is supported. Breaking: Added flatgeobuf::Error with more specific errors and use it where possible rather than geozero::GeozeroError. Added feature batching for HTTP client to optimize remote reading.

nyurik commented 7 months ago

I am a bit confused by the new error format (I'm not saying this is a blocker, just a bit uncertain why this pattern is used): usually, the more specific errors are wrapped by thiserror as #[error(transparent)] SubError(#[from] ErrorSubType) -- which allows everyone to use the top level error, and the ? would auto-convert the sub-error to top-error. Is this pattern no longer good?

michaelkirk commented 7 months ago

Can you be more specific about the alternative you’re implying Yuri? In what crate would these wrapped errors be defined? 

nyurik commented 7 months ago

Hmm, I guess this is once again that circular dependency issue.

Usually, I would expect this to be added to the GeozeroError enum:

#[derive(Error, Debug)]
pub enum GeozeroError {
    ...
    #[error(transparent)]
    FlatGeoBufError(#[from] flatgeobuf::Error),
}

Possibly with some conditional compilation. This way any code like this would not require any boxing (as you do in the tests) or the conversion function as in CLI:

        let ds = HttpFgbReader::open(&args.input).await?
michaelkirk commented 7 months ago

I'm not sure if that would be a good idea, but since we can't create a circular dependency anyway, I'm not sure it's worth going through the thought exercise.