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

Improve WkbWriter constructors #158

Merged
merged 3 commits into from
Aug 18, 2023
Merged

Improve WkbWriter constructors #158

merged 3 commits into from
Aug 18, 2023

Conversation

pka
Copy link
Member

@pka pka commented Aug 12, 2023

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

Follow-up to discussion with @Oreilles in #153 (comment)

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

@pka pka changed the title Wkb writer tweaks Improve WkbWriter constructors Aug 12, 2023
@nyurik
Copy link
Member

nyurik commented Aug 12, 2023

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

Oreilles commented Aug 12, 2023

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

nyurik commented Aug 13, 2023

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

pka commented Aug 13, 2023

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

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

This is done in #162

@pka pka marked this pull request as ready for review August 17, 2023 19:44
@pka pka requested a review from nyurik August 18, 2023 19:43
}

#[doc(hidden)]
// Temporary constructor. To be replaced with builder pattern.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eeek! ok :)

envelope_dims: CoordDimensions::default(),
extended_gpkg: false,
empty: false,
let srid = None;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two vars are not really needed - too little value, but i guess they are ok if we will switch to a builder soon

@pka pka merged commit b6e6361 into georust:main Aug 18, 2023
1 check passed
@pka pka deleted the wkb-writer-tweaks branch August 18, 2023 20:32
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

Successfully merging this pull request may close these issues.

4 participants