Save location details to Observer instance to match the docs.#522
Conversation
There was a problem hiding this comment.
The docstrings here should be changed. These are describing params as if the property could be set, but it cannot (which might wanted to be added, @bmorris3?).
Can you change the docstrings to match a getter property?
There was a problem hiding this comment.
Sure, I actually just copied it from the Observer class's own docstring. Is there a preferred style for getter property docstrings? (Otherwise I can try to find an example from the codebase).
There was a problem hiding this comment.
Ok I tried something, but let me know if you have any suggestions on it.
|
Thanks for the review @wtgee. I did not really find any examples of a Using the online editor so I hope it worked. |
|
Hey @bmorris3 , tagging you in case you haven't seen it. I would appreciate you enabling the workflow for this contribution (and let me know if you have any comments)! |
|
Hi @emirkmo! Apologies for taking my time to review this, it looks great. I'll work on getting the test failures to pass today. |
bmorris3
left a comment
bmorris3
left a comment
There was a problem hiding this comment.
Remaining failures are unrelated. Looks good!
|
Awesome. Glad it worked out. Happy I could contribute. Keep up the great work. |
to match the docs.
bmorris3
force-pushed
the
add_elevation
branch
from
17f789a to
e60ffe6
Compare
3 participants
Saves longitude latitude and elevation info to
Observerclass as properties to match the docs. Adds tests, docstrings, and updates changelog.Fixes #521 (I also noticed longitude and latitude were missing).
Discussion
The Linked PR addresses by adding
longitude,latitude, andelevationmethods with@propertydecorator that are re-calculated from the currentself.locationwhich is of typeastropy.coordinates.EarthLocation. So that updating the location also updates the parameters. Added a meaningful test that should fail if the underlying function fromastropychanges significantly.