-
-
Notifications
You must be signed in to change notification settings - Fork 167
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
ENH: Add _MotorPlots Inheritance to Motor Plots Classes #456
Conversation
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.
Removing 830 and adding 134 code lines, this is impressive. Good usage of parent-children classes arquitecture, loved that.
Regarding the "duplicated" code that you mentioned in the description. I don't think that is a huge problem. The methods are well documented so I don't see a big problem here, at least not for now.
We should do the same thing with the motor prints I guess.
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 am also very fond of this PR due to the code simplifications and architectural sense.
Just one comment that so that we pay attention to this mirroring of implementations:
- The code architecture is repeated (there will be three identical inheritances between Motor, MotorPrints and MotorPlots);
- I don't think this repetition is a good practice in the long term due to the fact that every change in the code hierarchy must be repeated twice (in prints and plots).
Nonetheless, the benefits of separating prints and plots currently outweighs these small issues by a good margin and this PR clearly simplifies the code. Well done.
@phmbressan do you see another alternative for the problem you mentioned? Out of curiosity... |
Pull request type
Checklist
black rocketpy/ tests/
) has passed locallypytest --runslow
) have passed locallyCurrent behavior
A lot of repeated code in the motors' plots classes
New behavior
_SolidMotorPlots
,_HybridMotorPlots
and_LiquidMotorPlots
now inherit_MotorPlots
Breaking change
Additional information
There are a lot of repeated code in
_SolidMotorPlots
and_HybridMotorPlots
I could not find a good solution for it. I tried using multiple inheritance in_HybridMotorPlots
but it went south really fastAlso, this is really useful for Motor Draws #436