-
Notifications
You must be signed in to change notification settings - Fork 94
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
[Future Spherical Class] Add SArc #478
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #478 +/- ##
==========================================
+ Coverage 94.27% 94.62% +0.35%
==========================================
Files 78 76 -2
Lines 12838 13701 +863
==========================================
+ Hits 12103 12965 +862
- Misses 735 736 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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 have a couple code cleanup suggestions, but otherwise this looks good to me. I assume all the math is correct.
class TestSArc(unittest.TestCase): | ||
"""Test SArc class.""" | ||
|
||
# TODO: Fixtures defined here? |
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.
You could define these as fixtures in the class, but considering this module is specific to testing the SArc class maybe it would be better to define them outside the class. But also...if we assume these classes/instances are immutable then you could also leave them like this.
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 will clean out the unit test when I will finalize it after the SPolygon behaviour.
I am not sure yet of all the behaviour ... and the current code helps me to work line by line
Co-authored-by: David Hoese <[email protected]>
Co-authored-by: David Hoese <[email protected]>
Co-authored-by: David Hoese <[email protected]>
Co-authored-by: David Hoese <[email protected]>
Looks like some of the github suggestions from my comments or your other recent changes have messed a few things up. Pre-commit and stickler and the website building are all mad. |
@djhoese I think now the code is ready for another round of review ;) |
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.
One more suggestion/question, sorry.
Looks like one of the tests is failing with the unstable dependencies. Not sure what's up with that. |
This PR implements the new SArc class
git diff origin/main **/*py | flake8 --diff