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

This PR rewrites to io.tess.TOI object using pydantic #425

Merged

Conversation

mwcraig
Copy link
Contributor

@mwcraig mwcraig commented Aug 13, 2024

Doing so has a couple of advantages:

  • The code is more compact
  • It is easy to serlialize to disk

Helps address, but does not complete fix the issue #188

@mwcraig mwcraig added this to the 2.0.0-beta milestone Aug 13, 2024
@mwcraig mwcraig requested a review from JuanCab August 13, 2024 14:32
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.44%. Comparing base (3f7ae4b) to head (07cc97f).
Report is 149 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
+ Coverage   78.91%   79.44%   +0.53%     
==========================================
  Files          30       30              
  Lines        3823     3800      -23     
==========================================
+ Hits         3017     3019       +2     
+ Misses        806      781      -25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@JuanCab JuanCab left a comment

Choose a reason for hiding this comment

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

I approve this, but would appreciate feedback on the questions.

Comment on lines +104 to 106
# Don't complain about connection errors
"raise requests.ConnectionError",
# Don't complain about script hooks
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to avoid errors during testing if the TESS servers are not available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and I wasn't sure how to test for that case...

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I had to read through it a bit, but pydantic classes do make some things a lot easier, like having properties. That said, I wonder, can you now set the values of those properties by calling them or is pydantic preventing that. Not a huge concern, but one nice feature of @property is allowing you to make read-only attributes... at the pydantic ones read-only once set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can now set them, but pydantic has a way of making them non-settable: https://docs.pydantic.dev/latest/concepts/models/#faux-immutability

Lmk if you want me to do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(merging for now, though, so I can open the next PR)

@mwcraig mwcraig merged commit c1f0947 into feder-observatory:main Aug 13, 2024
18 checks passed
@mwcraig mwcraig deleted the create-tess-target-source-list branch August 13, 2024 20:25
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.

2 participants