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

Fixed LLtoUTMUPSObject northing values in southern hemisphere. #26

Merged
merged 5 commits into from
Jan 9, 2019

Conversation

BernardIgiri
Copy link
Contributor

Northing values in the southern hemisphere are now correctly converted from Lat Lon to UTM. The unit tests have been updated to check northing values. In addition a isInUPSSpace convenience method is added with unit tests to help library users verify if a Lat Lon coordinate can be converted to UPS or not.

tests/tests.js Outdated Show resolved Hide resolved
tests/tests.js Show resolved Hide resolved
tests/tests.js Outdated Show resolved Hide resolved
@rymach
Copy link
Contributor

rymach commented Jan 9, 2019

Hero successful 🍾

@BernardIgiri
Copy link
Contributor Author

USNG4J equivalent PR: codice/usng4j#15

package.json Outdated Show resolved Hide resolved
} else {
let utmcoords = [0, 0, 0, 'N']
this.LLtoUTMwithNS(lat, lon, utmcoords)
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

This confused me a bit at first because I didn't realize utmcoords would be mutated by LLtoUTMwithNS.

Probably my own preference here, but I would prefer to avoid the mutation and have LLtoUTMwithNS return something instead.

That's outside this change though.

@@ -638,15 +638,27 @@ extend(Converter.prototype, {
}
},

isInUPSSpace(lat) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a typescript file we could define the type for this function to catch possible issues at compile time.

@andrewkfiedler andrewkfiedler merged commit 8e28107 into codice:master Jan 9, 2019
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.

7 participants