ENH: Add _MotorPlots Inheritance to Motor Plots Classes#456
ENH: Add _MotorPlots Inheritance to Motor Plots Classes#456Gui-FernandesBR merged 2 commits intodevelopfrom
Conversation
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
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.
phmbressan
left a comment
There was a problem hiding this comment.
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,_HybridMotorPlotsand_LiquidMotorPlotsnow inherit_MotorPlotsBreaking change
Additional information
There are a lot of repeated code in
_SolidMotorPlotsand_HybridMotorPlotsI could not find a good solution for it. I tried using multiple inheritance in_HybridMotorPlotsbut it went south really fastAlso, this is really useful for Motor Draws #436