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

switch long and lat for geopoint type #286

Merged
merged 1 commit into from Jun 6, 2017
Merged

Conversation

b-admike
Copy link
Contributor

@b-admike b-admike commented Jun 1, 2017

Description

MySQL expects reverse order of latitude and longitude from the way we use it in LoopBack, so switch the order when saving and loading Point spatial type we use for Point/GeoPoint.

Related issues

connect to #269
connect to https://github.com/strongloop-internal/scrum-apex/issues/239

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@kjdelisle
Copy link
Contributor

@bajtos Are we okay to merge this? I've opened the 5.x-beta branch as a way to land breaking changes but keep them off master for the time being.

@b-admike b-admike self-assigned this Jun 1, 2017
@bajtos
Copy link
Member

bajtos commented Jun 2, 2017

In the past, when we started to work on a new semver-major version, we created a new branch for the "latest" (4.x in this case) and kept "master" for the work on the upcoming major version. This way the git commit history remains linear. OTOH, when you fork off 5.x-beta branch and then merge it back to master later, then you will have two parallel lines in master history. There is also higher chance of getting merge conflicts, because by the time you merge 5.x-beta back into master, there may be other commits on master that are in conflict with 5.x-beta work.

Here is the description of the process we used for LoopBack 3.0: link to gist, see also our internal https://github.com/strongloop-internal/scrum-loopback/issues/601

Last but not least: we need to consider implications for long-term support, as described in http://loopback.io/doc/en/contrib/Long-term-support.html The important practical part is to ensure that each branch we are actively publishing from (3.x, 4.x, etc.) has a unique npm tag, and the tag "latest" is used by the branch containing the stable GA version. For example, strong-remoting's master branch is publishing "3.x" versions tagged as "latest", while the 2.x branch is publishing "2.x" versions tagged as "lts". The section [Publishing pre-release versions] in my gist contains additional information.

This is the process we used in LoopBack, it worked pretty well for us IMO. Of course, feel free to choose a different process if it works better for your squad/team.

@bajtos
Copy link
Member

bajtos commented Jun 2, 2017

See npm/npm#6778 for an explanation why we need different npm tags for different release lines.

@b-admike
Copy link
Contributor Author

b-admike commented Jun 2, 2017

I believe the new test was passing with the old implementation too, because the (wrong) order was used both for loading and saving. It would be nice to have a test that actually fails in the old implementation, I think it has to invoke custom SQL command using MySQL's Point features.

@bajtos PTAL. I've added more checks which make use of MySQL's Point functions and would fail in the old way we saved and loaded the GeoPoint datatype.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I don't see any obvious problems. LGTM (FWIW).

};
var xcor, ycor;
City.create(city1, function(err, res) {
assert.ok(!err);
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, we use the following pattern in other LoopBack projects - it produces more helpful failure messages:

- assert.ok(!err);
+ if (err) return done(err);

@kjdelisle
Copy link
Contributor

kjdelisle commented Jun 5, 2017

@b-admike Before this merges, don't forget to change the target branch based on your discussion with @bajtos, and to make sure you've rebased accordingly!

@b-admike b-admike changed the base branch from 5.x-beta to master June 6, 2017 12:51
MySQL expects reverse order of latitude and longitude
from the way we use it in LoopBack, so switch the order
when saving and loading Point spatial type we use for
Point/GeoPoint.
@b-admike b-admike merged commit 4077ae8 into master Jun 6, 2017
@b-admike b-admike removed the review label Jun 6, 2017
@b-admike b-admike deleted the fix/geopoint-orientation branch June 6, 2017 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants