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
DM-11473: Add SpherePoint(long, lat, unit) constructor #281
Conversation
Remove SpherePoint(double const lonLatRad[2]) constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One nit-pick.
tests/test_spherePoint.py
Outdated
@@ -128,6 +128,14 @@ def testCopyConstructor(self): | |||
spcopy = SpherePoint(sp) | |||
self.assertEqual(sp, spcopy) | |||
|
|||
def testDoubleConstructor(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DoubleConstructor
is potentially ambiguous. Maybe SharedUnitConstructor
?
@@ -63,7 +63,8 @@ Angle haversine(Angle const& deltaLon, Angle const& deltaLat, double cosLat1, do | |||
} | |||
} // end namespace | |||
|
|||
SpherePoint::SpherePoint(double const lonLatRad[2]) : _longitude(lonLatRad[0]), _latitude(lonLatRad[1]) {} | |||
SpherePoint::SpherePoint(double longitude, double latitude, AngleUnit units) | |||
: _longitude(longitude * units), _latitude(latitude * units) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't believe I overlooked this: please add input validation for longitude
and latitude
(as implied by the documentation) and test it. The easiest way to implement input validation is to delegate to the (Angle, Angle)
constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this the first time. I'll add shortly it in a new ticket.
0b68089
to
4934e7e
Compare
No description provided.