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

Throw exception if calculation time is out of range #356

Merged
merged 2 commits into from
Apr 25, 2020

Conversation

Deuchnord
Copy link
Contributor

@Deuchnord Deuchnord commented Apr 13, 2020

Add support for the OutOfRangeError exception thrown by JplEphem in brandon-rhodes/python-jplephem#36.

Basically, we catch the raised exception on JplEphem's side and re-throw a Skyfield-specific one with the times converted in Time objects.

Fix #319

@Deuchnord Deuchnord marked this pull request as ready for review April 13, 2020 10:51
@brandon-rhodes brandon-rhodes self-assigned this Apr 13, 2020
@brandon-rhodes
Copy link
Member

Thanks, hopefully I'll have a chance to take a look later this week!

Copy link
Member

@brandon-rhodes brandon-rhodes left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for doing this work!

  1. I've asked for a small code change.
  2. And also an API/feature tweak.
  3. Also: could you add a unit test for the feature? You'll want to create a new file test_jpllib.py since it's a test specific to this suite.

Thanks, I look forward to merging this!

skyfield/jpllib.py Outdated Show resolved Hide resolved
skyfield/jpllib.py Outdated Show resolved Hide resolved
skyfield/jpllib.py Outdated Show resolved Hide resolved
@Deuchnord
Copy link
Contributor Author

Hi @brandon-rhodes, sorry for the late, didn't have much time this week to fix the review.

Also: could you add a unit test for the feature? You'll want to create a new file test_jpllib.py since it's a test specific to this suite.

I'm not sure how I should proceed for that… at first sight, looks like I should mock the JplLib library, but I don't know the tests framework you're using… do you have any doc for it?

@brandon-rhodes brandon-rhodes merged commit 468b868 into skyfielders:master Apr 25, 2020
@brandon-rhodes
Copy link
Member

Thanks for contributing this! The feature should appear in the next release of Skyfield.

@Deuchnord Deuchnord deleted the out-of-range-time-error branch April 26, 2020 09:47
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.

Add support for jplephem's new OutOfRangeError exception
2 participants