-
Notifications
You must be signed in to change notification settings - Fork 12
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
Move table representation builder #420
Move table representation builder #420
Conversation
Marked as a draft to make sure we get #419 in first |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #420 +/- ##
==========================================
+ Coverage 78.87% 78.91% +0.04%
==========================================
Files 29 30 +1
Lines 3815 3823 +8
==========================================
+ Hits 3009 3017 +8
Misses 806 806 ☔ View full report in Codecov by Sentry. |
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.
Interesting fix. I can't claim to understand the tox
aspects of this, but the logic seemed sound.
.github/workflows/build.yml
Outdated
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'm not fully conversant in or even have any experience at all withtox. So I did look over the tox stuff and understand the idea, although I can't really be relied on to catch any bugs in it. :(
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 was marginally surprised the tox changes worked 🤣
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.
Nice solution to the testing problem.
The merge-base changed after approval.
Co-authored-by: Juan Cabanela <[email protected]>
@JuanCab -- is this OK to merge once tests pass? |
Yep |
Fixes #408.
The root cause of #408 was that the mechanism for allowing an astropy
Table
to read/write our custom models was built into the model classes and not run unless one instantiated each of the models. As a result, if you tried reading aPhotometryData
table before you instantiated a camera and an observatory then the read would fail.This PR fixes the issue by ensuring that the code to add the table representations is run when stellarphot is imported.
This issue is hard to test for because if any code anywhere in the test suite makes a
Camera
and anObservatory
hen the test passes. To ensure that we are testing for this failure I added a special tox case for it and added a workflow that runs that test.