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

GeoJson Compliant Decoding #33

Closed
jtbaker opened this issue Feb 3, 2021 · 6 comments · May be fixed by #34
Closed

GeoJson Compliant Decoding #33

jtbaker opened this issue Feb 3, 2021 · 6 comments · May be fixed by #34

Comments

@jtbaker
Copy link

jtbaker commented Feb 3, 2021

Would the team be open to adding a flag or options argument to the decode function to parameterize that x, y encoding for the coordinate pairs per the GeoJSON spec is needed and not the default y, x encoding. I can see there are low level bindings in various languages implemented for this and understand it's not a small task, but I think it would be quite helpful for many use cases. If you think that sort of function signature would work for decode or would consider a separate decodeToGeoJson function, I'd be happy to PR it for the JavaScript implementation.

@jtbaker jtbaker changed the title GeoJson Compliant Encoding GeoJson Compliant Decoding Feb 3, 2021
@VeaaC
Copy link
Contributor

VeaaC commented Feb 3, 2021

In general it could make sense to have the APIs a bit more flexible, as long as they stay easy to use (e.g. it should always be clear which kind of input is given).

Would you mind crafting an example (in one language), how you would imagine such an extension to look like?

@jtbaker
Copy link
Author

jtbaker commented Feb 3, 2021

Hi @VeaaC, thanks for considering the idea. I've created PR #34 for the JS implementation. Let me know what your thoughts are.

@VeaaC
Copy link
Contributor

VeaaC commented Feb 3, 2021

@roshats @lene what do you think?

Adding support for something like this could be done in several ways:
A) add descriptors on how to interpret input/output
B) add separate encode/decode function (encode_from_geojson / decode_into_geojson)
C) let the user convert coordinate types // <-- current API

Also consider how that would impact the 3D version (which already describes the variable 3rd dimension with a type). E.g. the Rust API looks like this right now:

pub enum Polyline {
    Data2d {
        coordinates: Vec<(f64, f64)>,
        precision2d: Precision,
    },
    Data3d {
        coordinates: Vec<(f64, f64, f64)>,
        precision2d: Precision,
        precision3d: Precision,
        type3d: Type3d,
    },
}

impl Polyline {
    pub fn encode(&self) -> Result<String, Error> { ...  }
    pub fn decode<S: AsRef<str>>(encoded: S) -> Result<Self, Error> { ... }

@lene
Copy link
Contributor

lene commented Feb 4, 2021

I am not keen on possibility A, for a number of reasons. First, this is a reference implementation for encoding Flexible Polylines (even if it has turned into a library in the meantime, it still serves as reference implementation) and for that reason it should do just de- and encoding. Then, if we add GeoJSON conversion for one language, we would need to do it for all supported languages and I don't see anyone taking the time for that.

I could go with B, preferably keeping the encode_from_geojson / decode_to_geojson functions in different source files, so as to not confuse people looking for the reference implementation with code that is irrelevant to them. But I think my favored approach would be keeping GeoJSON de-/encoding out of this repository entirely and starting a new repo, e.g. "geojson-flexible-polyline" for that purpose. That way we have proper separation of responsibilities and avoid feature creep.

Thoughts? Anyone?

@jtbaker
Copy link
Author

jtbaker commented Feb 8, 2021

As a user, I think it makes much more semantic sense to have each of these functions contained together in a single module, since they fundamentally do the basic operations. A separate repo for each function doesn't seem quite idiomatic to me. But maybe those belong in a separate library repo as you mentioned, and not this reference implementation.

import { decode, decode_to_geojson } from "@here/flexible-polyline"

vs.

import { decode } from "@here/flexible-polyline"
import { decode_to_geojson } from "@here/geojson-flexible-polyline"

I'll respect whatever your team decides re: this issue, just wanted to give some feedback on a helpful use case for how my org is using some of the HERE APIs and needing to interface the outputs with other SDKs. Thanks for your time.

@VeaaC VeaaC closed this as completed Jun 6, 2024
@VeaaC
Copy link
Contributor

VeaaC commented Jun 6, 2024

Closed this issue, it was accidentally left open for all this time :)

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 a pull request may close this issue.

3 participants