-
-
Notifications
You must be signed in to change notification settings - Fork 208
Enh/custom warning no motor parachute aerosurface #871
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
base: master
Are you sure you want to change the base?
Enh/custom warning no motor parachute aerosurface #871
Conversation
Added a placeholder in the [Unreleased] section for the upcoming feature to add custom warnings when a rocket has no motors, parachute, or AeroSurface. See RocketPy-Team#285
This enhancement adds a warning when a Rocket object has no motors, parachutes, or AeroSurface components. It notifies the user so that they can add missing components before running simulations. See RocketPy-Team#285
| """Tests the _check_missing_components method for a complete Rocket.""" | ||
| # Call directly — no warnings expected | ||
| calisto_robust._check_missing_components() | ||
| # If any warning occurs, pytest will fail automatically |
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.
are you sure?
# If any warning occurs, pytest will fail automatically
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.
Thank you for the review!You're correct.By default, pytest only displays warnings but doesn't fail.I will updated the test.
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.
changelog seems weird @phmbressan
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.
Thanks for your feedback. Could you please clarify which part of the changelog seems weird? I want to make sure my addition follows the proper format.
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.
your added line is listed along together other changes that have already been deployed
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.
changelog seems weird @phmbressan
I can double-check later, but this branch might not be up to date with develop (v1.11), which is why the CHANGELOG seems outdated. Could you @C8H10O2 try rebasing this branch with the current develop so that all conflicts are sorted out?
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.
changelog seems weird @phmbressan
I can double-check later, but this branch might not be up to date with develop (v1.11), which is why the CHANGELOG seems outdated. Could you @C8H10O2 try rebasing this branch with the current develop so that all conflicts are sorted out?
Understood, I’ll take care of it. Thanks for pointing it out.
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.
a rocket without parachute is something quite common.
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.
Following up on issue #285 and your comment:
I wanted to clarify: for rockets without parachutes, should the custom warning be changed to a notice/info message instead, or should it only trigger under specific conditions?
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.
we should never raise an warning for "parachuteless rockets", so to say
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest tests -m slow --runslow) have passed locallyCHANGELOG.mdhas been updated (if relevant)Current behavior
There is no test for the _check_missing_components().
New behavior
This PR adds unit tests to verify the warning behavior of the
_check_missing_components()method.Specifically, it includes three test cases:
These tests are defined in
tests/unit/test_rocket.py.Breaking change