Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changelog seems weird @phmbressan

Copy link

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.

Copy link
Member

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

Copy link
Collaborator

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?

Copy link

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.

Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ Attention: The newest changes should be on top -->

### Added

- ENH: Custom Exception errors and messages [#285](https://github.com/RocketPy-Team/RocketPy/issues/285)

### Changed

### Fixed
Expand Down
26 changes: 26 additions & 0 deletions rocketpy/rocket/rocket.py
Copy link
Member

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.

Copy link

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:

#285 (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?

Copy link
Member

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

Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,32 @@ def __init__( # pylint: disable=too-many-statements
self.prints = _RocketPrints(self)
self.plots = _RocketPlots(self)

def _check_missing_components(self):
"""Check if the rocket is missing any essential components and issue a warning.

This method verifies whether the rocket has the following key components:
- motor
- aerodynamic surface(s)

If any of these components are missing, a single warning message is issued
listing all missing components. This helps users quickly identify potential
issues before running simulations or analyses.

Notes:
- The warning uses Python's built-in `warnings.warn` function.
Comment on lines +398 to +399
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring is missing a 'Returns' section. According to NumPy docstring format, if a method doesn't return anything, it should include 'Returns\n-------\nNone' to be explicit. This aligns with RocketPy's requirement for comprehensive NumPy-style docstrings.

Suggested change
Notes:
- The warning uses Python's built-in `warnings.warn` function.
Notes
-----
- The warning uses Python's built-in `warnings.warn` function.
Returns
-------
None

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True.

"""
missing_components = []
if isinstance(self.motor, EmptyMotor):
missing_components.append("motor")
if not self.aerodynamic_surfaces or len(self.aerodynamic_surfaces) == 0:
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition not self.aerodynamic_surfaces already covers empty lists, making the explicit len(self.aerodynamic_surfaces) == 0 check redundant. Simplify to just if not self.aerodynamic_surfaces:.

Suggested change
if not self.aerodynamic_surfaces or len(self.aerodynamic_surfaces) == 0:
if not self.aerodynamic_surfaces:

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true

missing_components.append("aerodynamic surfaces")

if missing_components:
component_list = ", ".join(missing_components)
warnings.warn(
f"[WARNING] Rocket has no {component_list} defined.", UserWarning
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The '[WARNING]' prefix in the warning message is redundant since warnings.warn() already identifies messages as warnings. Remove the prefix to avoid duplication: f'Rocket has no {component_list} defined.'

Suggested change
f"[WARNING] Rocket has no {component_list} defined.", UserWarning
f"Rocket has no {component_list} defined.", UserWarning

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true

)

@property
def nosecones(self):
"""A list containing all the nose cones currently added to the rocket."""
Expand Down
37 changes: 36 additions & 1 deletion tests/unit/rocket/test_rocket.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@


@patch("matplotlib.pyplot.show")
def test_elliptical_fins(mock_show, calisto_robust, calisto_trapezoidal_fins): # pylint: disable=unused-argument
def test_elliptical_fins(
mock_show, calisto_robust, calisto_trapezoidal_fins
): # pylint: disable=unused-argument
test_rocket = calisto_robust
calisto_robust.aerodynamic_surfaces.remove(calisto_trapezoidal_fins)
test_rocket.add_elliptical_fins(4, span=0.100, root_chord=0.120, position=-1.168)
Expand Down Expand Up @@ -370,6 +372,39 @@ def test_add_motor(calisto_motorless, cesaroni_m1670):
assert center_of_mass_motorless is not center_of_mass_with_motor


def test_check_missing_all_components(calisto_motorless):
"""Tests the _check_missing_components method for a Rocket with no components."""
with pytest.warns(UserWarning) as record:
calisto_motorless._check_missing_components()

assert len(record) == 1
msg = str(record[0].message)
assert "motor" in msg
assert "aerodynamic surfaces" in msg


def test_check_missing_some_components(calisto):
"""Tests the _check_missing_components method for a Rocket missing some components."""
calisto.parachutes = []
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test modifies calisto.parachutes = [] on line 388, but the _check_missing_components() method doesn't check for parachutes—only motor and aerodynamic surfaces. This modification is unnecessary and may confuse future maintainers about what the method actually validates. Remove line 388.

Suggested change
calisto.parachutes = []

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true

calisto.aerodynamic_surfaces = []

with pytest.warns(UserWarning) as record:
calisto._check_missing_components()

assert len(record) == 1
msg = str(record[0].message)
assert "aerodynamic surfaces" in msg


def test_check_missing_no_components_missing(calisto_robust):
"""Tests the _check_missing_components method for a complete Rocket."""
# Call directly — no warnings expected
with pytest.warns(None) as record:
calisto_robust._check_missing_components()
# If any warning occurs, pytest will fail automatically
Copy link
Member

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

Copy link
Author

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.

assert len(record) == 0


def test_set_rail_button(calisto):
rail_buttons = calisto.set_rail_buttons(0.2, -0.5, 30)
# assert buttons_distance
Expand Down
Loading