Adds Ewkt dialect - Githubissues

georust / geozero

Zero-Copy reading and writing of geospatial data.

Apache License 2.0

322 stars 30 forks source link

Adds Ewkt dialect #155

Closed Oreilles closed 11 months ago

Oreilles commented 11 months ago

This PR adds:

enum WktDialect with options WktDialect::Wkt and WktDialect::Ewkt to wkt module
struct EwktString(pub String)
fn to_ewkt(&self, srid: Option<i32>) -> Result<String> to trait ToWkt
fn to_wkt_dialect(&self, dialect: WktDialect, dims: CoordDimensions, srid: Option<i32>) -> Result<String> to trait ToWkt

The trait FromWkb is implemented for EwktString so that the srid is inferred from the Wkb stream if available. The later is made possible by implementing GeomProcessor::srid for WktWriter and calling processor.srid(info.srid) in process_wkb_geom.

Side notes:

This PR only implements Ewkt write. Ewkt read should probably be implemented in the wkt crate.
Should we add ProcessToCsv::to_csv_ewkt and/or ProcessToCsv::to_csv_dialect?

nyurik commented 11 months ago

TBH, I am not too happy about changing all of the APIs to force the user to always be WKT dialect aware. I would much rather keep the existing API, plus perhaps add a new constructor, e.g. keep the WktWriter::new(w: impl Write), and add another one like WktWriter::with_dialect(w: impl Write, dialect: WktDialect)?

Oreilles commented 11 months ago

Sure, I changed the signature of the constructor for the sake of consistency with WkbWriter but I would definetly understand that an API change would not be acceptable.

nyurik commented 11 months ago

oh, i didn't realize we already did it that way with WkbWriter... I'm a bit on a fence on this one then - ideally I would want Wkb to also be done the same way - WkbWriter::with_dialect(...), but I will let @pka to chime in on this one

Oreilles commented 11 months ago

Thanks for the review, I ran fmt and took example on the linked PR to remove the WktReader lifetime 👍

nyurik commented 11 months ago

@Oreilles thx! Do you need help merging main branch into your PRs?

Oreilles commented 11 months ago

I just rebased onto main and solved the few conflicts.

pka commented 11 months ago

After discussing the PR with @nyurik, I'm also for keeping the WktWriter::new signature and adding a second method WktWriter::with_dialect. For WkbWriter the dialect is essential, but WKT users usually don't care and I don't want to force them to think about it. What's your use case of using EWKT? Can't remember anyone mentioning it for a long time.

Oreilles commented 11 months ago

After discussing the PR with @nyurik, I'm also for keeping the WktWriter::new signature and adding a second method WktWriter::with_dialect. For WkbWriter the dialect is essential, but WKT users usually don't care and I don't want to force them to think about it.

That's understandable, I'll do the modification 👍

What's your use case of using EWKT ?

Even though it's not a standard, thanks to PostGIS success it's still quite common and (AFAIAA) is the only human-friendly geometry format that allows SRID declaration. It will be used as a user input format in a product I'm working on.

nyurik commented 11 months ago

I made a few more suggestions for this PR in https://github.com/Oreilles/geozero/pull/2

rename to_wkt_dialect -> to_wkt_with_opts
make WktWriter content private! It's ridiculous to have write access to a writer from outside
introduce WktWriter::with_dims to allow creation of a writer with the needed params