-
-
Notifications
You must be signed in to change notification settings - Fork 174
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
MNT: Refactor AeroSurfaces
#634
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #634 +/- ##
===========================================
- Coverage 73.92% 73.90% -0.03%
===========================================
Files 70 79 +9
Lines 10032 10070 +38
===========================================
+ Hits 7416 7442 +26
- Misses 2616 2628 +12 ☔ 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.
Good job.
It will be much easier to add new AeroSurface
now.
The flight.py
file is the only one that warns me a bit. Please double check this file to ensure that you are not making any bad operation.
rocketpy/rocket/aero_surface.py
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.
This file deletion will probably create merge conflicts with the following PR: #597
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.
classes Fins
and AeroSurface
are abstract classes and therefore cannot be instantiated.
They are only exposed here if the user decides to create a new class based on them.
With that being said, I think we should avoid re-exposing them in the init files.
I don't want someone doing something like this:
from rocketpy import Fins
fins = Fins(...) # raises an error!!
An experienced user would still be able to import the class using its absolute path, like this:
from rocketpy.rocket.aero_surface.fins import Fins,
class NewFins(Fins):
...
Pull request type
Checklist
black rocketpy/ tests/
) has passed locallypytest tests -m slow --runslow
) have passed locallyCHANGELOG.md
has been updated (if relevant)Changes
AeroSurface
class into a different fileAeroSurface
classBreaking change