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

Merge Coordinate into Point type #16

Closed
wants to merge 2 commits into from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Oct 22, 2015

This simplifies the Point API and removes an unnecessary abstraction level.

Resolves #15

This simplifies the Point API and removes an unnecessary abstraction level.

Resolves georust#15
@frewsxcv
Copy link
Member

@georust/core Considering this is a breaking change, might be good to get some feedback here

I'm personally fine with this change

@frewsxcv
Copy link
Member

So after a couple days of thinking, here are my thoughts:

  1. If we intend to keep the existing hierarchy (e.g. where MultiPolygon contains Polygons), I'm still in favor of this change
  2. Is there any reason why a MultiPolygon should contain multiple Polygons? I'm not sure why we designed it that way, but this goes against many other geographic formats (e.g. GeoJSON, WKT) in addition to the Simple Feature Access standard

I'm slightly in favor of going to down the latter route. What do you think @Turbo87 ?

@frewsxcv frewsxcv mentioned this pull request Oct 25, 2015
@calvinmetcalf
Copy link
Member

@frewsxcv It was designed around geojson which is by far the most pragmatic geospatial format

@sunng87
Copy link
Member

sunng87 commented Oct 27, 2015

JTS and libgeos has separated Coordinate and Point. GeoJSON has a Position concept. Personally I'm not quite sure about the benefit and risk to combine Point and Coordinate.

@Turbo87
Copy link
Member Author

Turbo87 commented Oct 27, 2015

@frewsxcv I have not thought about MultiPolygons yet and never actually used them, but I also don't see how that is related to Points and Coordinates.

@calvinmetcalf as mentioned in #15 already: modeling after GeoJSON can work, but since JSON doesn't have a proper type system we can actually do much better in Rust. GeoJSON needs both Point and Coordinate because of the type attribute for the Point feature, but since Rust has a type system there is basically no difference between the two anymore.

@sunng87 the advantage is a simpler API and I honestly don't see any risks or disadvantages at this point. If I'm missing something then please tell me.

@frewsxcv
Copy link
Member

Is there any reason why a MultiPolygon should contain multiple Polygons? I'm not sure why we designed it that way, but this goes against many other geographic formats (e.g. GeoJSON, WKT) in addition to the Simple Feature Access standard

I mentioned this, because if we decide to go this route and not make MultiPolygon not consist of Polygons, this pull request should probably not merged

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 26, 2015

closing due to inactivity

@Turbo87 Turbo87 closed this Nov 26, 2015
@Turbo87 Turbo87 deleted the remove-coordinate branch February 16, 2017 11:12
nyurik pushed a commit to nyurik/geo that referenced this pull request Mar 19, 2022
Move tests to their appropriate submodule
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