Minor adjustment on error semantics and model business rules #42
Conversation
…cone mandatory parameters
WalkthroughThis pull request updates error messages in simulation methods across several controllers to indicate that parameters may be physically incoherent. The Rocket model is modified so that its Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
GabrielBarberini
requested review from
phmbressan,
shantanu-sardesai and
yogirajgutte
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
lib/controllers/environment.py (1)
267-267: LGTM! Error message improvement enhances debugging.The updated error message now provides clearer context about simulation failures by indicating that parameters may not be physically coherent. This aligns well with the PR's objective to improve error semantics.
Consider adding a link to documentation about physical coherence requirements in the error message:
- detail=f"Failed to simulate environment, parameters may contain data that is not physically coherent: {exc_str}", + detail=f"Failed to simulate environment, parameters may contain data that is not physically coherent (see https://docs.rocketpy.org/parameters): {exc_str}",lib/controllers/rocket.py (1)
282-282: LGTM! Consistent error message improvement.The error message update maintains consistency with other controllers while providing clearer context about simulation failures.
For consistency with the environment controller, consider adding the same documentation link:
- detail=f"Failed to simulate rocket, parameters may contain data that is not physically coherent: {exc_str}", + detail=f"Failed to simulate rocket, parameters may contain data that is not physically coherent (see https://docs.rocketpy.org/parameters): {exc_str}",tests/test_routes/conftest.py (2)
83-94: LGTM! Consider adding docstring.The
stub_nose_conefixture provides a minimal validNoseConeinstance for testing.Add a docstring to document the fixture's purpose and usage:
@pytest.fixture def stub_nose_cone(): + """Provides a minimal valid NoseCone instance for testing. + + Returns: + dict: JSON representation of a NoseCone instance + """ nose_cone = NoseCone(
96-108: LGTM! Consider adding docstring.The
stub_finsfixture provides a minimal validFinsinstance for testing.Add a docstring to document the fixture's purpose and usage:
@pytest.fixture def stub_fins(): + """Provides a minimal valid Fins instance for testing. + + Returns: + dict: JSON representation of a Fins instance + """ fins = Fins(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/controllers/environment.py(1 hunks)lib/controllers/flight.py(1 hunks)lib/controllers/motor.py(1 hunks)lib/controllers/rocket.py(1 hunks)lib/models/rocket.py(1 hunks)requirements-dev.txt(1 hunks)tests/test_routes/conftest.py(3 hunks)tests/test_routes/test_rockets_route.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/test_routes/test_rockets_route.py
✅ Files skipped from review due to trivial changes (2)
- lib/controllers/flight.py
- lib/controllers/motor.py
🧰 Additional context used
📓 Learnings (1)
tests/test_routes/conftest.py (1)
Learnt from: GabrielBarberini
PR: RocketPy-Team/Infinity-API#41
File: tests/test_routes/test_flights_route.py:122-125
Timestamp: 2024-12-07T11:50:08.415Z
Learning: In the project's pytest test files (e.g., `tests/test_routes/test_flights_route.py`), fixtures like `stub_rocket` and `stub_flight` are function-scoped and can be safely modified within tests without causing side effects.
🔇 Additional comments (4)
lib/models/rocket.py (1)
36-37: LGTM! Note: Breaking change detected.Making
noseandfinsrequired parameters aligns with the PR objectives to modify model business rules. This change improves type safety by ensuring these critical components are always specified.However, this is a breaking change that will require updates to all code creating
Rocketinstances.Please ensure that:
- The package version is bumped according to semver
- The breaking changes are documented in the changelog
- All
Rocketinstance creations are updated across the codebaseRun this script to find potential instances requiring updates:
tests/test_routes/conftest.py (2)
7-7: LGTM!The new imports align with the changes in the
Rocketmodel requiringFinsandNoseCone.
111-123: LGTM! The fixture correctly uses the new required parameters.The
stub_rocketfixture has been properly updated to include the now-requirednoseandfinsparameters using the new fixtures.requirements-dev.txt (1)
4-5: LGTM! Development dependencies added.The addition of
pytestandhttpxas development dependencies is appropriate:
pytestis required for the test fixtureshttpxis useful for testing HTTP endpointsRun this script to check for version constraints and potential conflicts:
✅ Verification successful
Development dependencies verified
The output confirms that bothpytestandhttpxare correctly defined with no version conflicts. The dry run installation and version checks indicate thatpytest(8.3.4) andhttpx(0.28.1) match the expected latest versions with their dependency constraints properly satisfied.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check package versions and potential conflicts # Install packages and check for conflicts pip install pytest httpx --dry-run # Get latest versions for comparison echo "Latest versions:" pip index versions pytest pip index versions httpxLength of output: 3857
There was a problem hiding this comment.
Any reason for not allowing rockets without fins or nose?
There was a problem hiding this comment.
Its a FUP from the recent debug session we had. We understand that rockets without fins or nose are a minor use-case but a major bug raiser. Since changing from mandatory to optional or vice-versa is an easy task we decided to keep it more restrictive for now.
The bug itself we were tracking is not on the API side tho. @phmbressan will probably share more details about this in the upcoming libdev meetings.
Summary by CodeRabbit