georust / geozero

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

Improve WkbWriter constructors #158

Closed pka closed 10 months ago

pka commented 11 months ago

This PR adresses #156 (first commit) and should fix different dimensions in reader and writer (second commit).

Follow-up to discussion with @Oreilles in https://github.com/georust/geozero/issues/153#issuecomment-1675943714

Instead of the ugly with_extended_opts constructor we should probably use a builder pattern.

nyurik commented 11 months ago

I think this is a good direction, thx! Would it make sense to perhaps simplify all readers/writers to own underlying impl Read/impl Write? Same as std::io::BufWriter and others? The wrappers would still allow access to the underlying inner reader/writer, same as buffer - with get_ref and get_mut

Oreilles commented 11 months ago

Thanks for solving this, I was about to go at it when I saw your PR 🙂

Regarding the constructor issue: I don't know if that's a common pattern in rust, but in Typescript you often create functions accepting a single parameter object containing all the options, rather than one function argument for each parameter. It makes it easier to merge default values and reduce friction with adding new options. It has the disadvantage of being a bit more verbose though.

In this case, it could look something like this (untested):

#[derive(Default)]
pub struct WkbWriterParams {
    pub dialect: WkbDialect,
    pub dims: CoordDimensions,
    pub srid: Option<i32>,
    pub envelope: Vec<f64>,
    pub envelope_dims: CoordDimensions,
    pub extended_gpkg: bool,
}

impl<'a, W: Write> WkbWriter<'a, W> {
    pub fn new(out: &'a mut W, params: WkbWriterParams) -> WkbWriter<'a, W> {
        WkbWriter {
            dialect: params.dialect,
            dims: params.dims,
            srid: params.srid,
            envelope: params.envelope,
            envelope_dims: params.envelope_dims,
            extended_gpkg: params.extended_gpkg,
            empty: false,
            endian: scroll::LE,
            dialect,
            first_header: true,
            geom_state: GeomState::Normal,
            out,
        }
    }
}

// Or even just: 
impl<'a, W: Write> WkbWriter<'a, W> {
    pub fn new(out: &'a mut W, params: WkbWriterParams) -> WkbWriter<'a, W> {
        WkbWriter {
            params,
            empty: false,
            endian: scroll::LE,
            dialect,
            first_header: true,
            geom_state: GeomState::Normal,
            out,
        }
    }
}

which you would then call as such:

let writer = WkbWriter::new(out, WkbWriterParams { srid: Some(4326), Default::default() })
nyurik commented 11 months ago

Rust usually uses builder pattern as it is far more concise and does not break when options are changed, e.g. this guide. The biggest unknown is if the builder should return &mut Self or Self on each setter

pka commented 10 months ago

I'm hesitating to put too much work into this mostly internal API. We could also implement Default for writer structs instead of having with_extended_opts or builder functions. But implementingDefault isn't easy because of out: &'a mut W. So yes, we should probably switch to out: W.

michaelkirk commented 10 months ago

So yes, we should probably switch to out: W.

This is done in #162