Skip to content

GeoPoint -> LatLng#466

Merged
ajfriend merged 77 commits into
uber:masterfrom
ajfriend:latlng_code
Jun 5, 2021
Merged

GeoPoint -> LatLng#466
ajfriend merged 77 commits into
uber:masterfrom
ajfriend:latlng_code

Conversation

@ajfriend

@ajfriend ajfriend commented May 19, 2021

Copy link
Copy Markdown
Collaborator

To keep the number of changes under control, I've tried to focus this PR around the renaming of GeoPoint to LatLng and generally using lng instead of lon throughout the codebase. I list a few other potential related changes that we might also want to consider, but I'm proposing we do those in a separate PR.

Renaming

  • GeoPoint to LatLng
  • LinkedGeoPoint to LinkedLatLng (is this one a bit weird?)
  • pointToCell to latLngToCell
  • cellToPoint to cellToLatLng
  • vertexToPoint to vertexToLatLng
  • generally use lng instead of lon throughout the codebase

Work for follow-up PRs

  • LinkedGeoPolygon?
  • LinkedGeoLoop?
  • GeoLoop?
  • GeoPolygon?
  • GeoMultiPolygon?
  • pointDist* to sphereDist*?
  • other various uses of "point" and "geo" throughout the codebase?

@ajfriend
ajfriend marked this pull request as draft May 19, 2021 02:46
@coveralls

coveralls commented May 19, 2021

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.0003%) to 98.991% when pulling b8e018e on ajfriend:latlng_code into 9ab1053 on uber:master.

@ajfriend

Copy link
Copy Markdown
Collaborator Author

@isaacbrodsky, I'm not sure why the website was breaking and this fixed it, as I'm not really sure what would have changed to break things...

Also, I updated the js live examples to use the 4.0 names, but those will be broken on the website until we release 4.0 for JS.

Comment thread website/README.md Outdated
@ajfriend
ajfriend requested a review from isaacbrodsky May 31, 2021 02:34

@nrabinowitz nrabinowitz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These names are a huge improvement IMO. One nice thing too is that it makes it easy for the bindings to additionally offer lngLatToCell or cellToLngLat in case the caller prefers.

Comment thread src/apps/applib/lib/kml.c Outdated
Comment thread src/apps/testapps/mkRandGeoBoundary.c
Comment thread src/apps/testapps/testH3CellArea.c
Comment thread src/apps/testapps/testH3Index.c Outdated
Comment thread src/h3lib/lib/faceijk.c
/** @brief icosahedron face centers in lat/lon radians */
const GeoPoint faceCenterGeo[NUM_ICOSA_FACES] = {
/** @brief icosahedron face centers in lat/lng radians */
const LatLng faceCenterGeo[NUM_ICOSA_FACES] = {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This one's super helpful, since we declare the data below positionally

Comment thread website/README.md Outdated
List<Long> polyfill(List<GeoCoord> points, List<List<GeoCoord>> holes, int res);
List<String> polyfillAddress(List<GeoCoord> points, List<List<GeoCoord>> holes, int res);
List<Long> polyfill(List<LatLng> points, List<List<LatLng>> holes, int res);
List<String> polyfillAddress(List<LatLng> points, List<List<LatLng>> holes, int res);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These Java ones are anticipating that the binding will change, not sure if that makes sense at this point?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should use consistent terms in the bindings as well, so this makes sense to me.

Comment thread website/docs/core-library/latLngToCellDesc.md Outdated
@ajfriend
ajfriend merged commit 32b6e07 into uber:master Jun 5, 2021
@ajfriend
ajfriend deleted the latlng_code branch June 5, 2021 02:21
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.

5 participants