Skip to content

Conversation

@Marchma0
Copy link

@Marchma0 Marchma0 commented Nov 4, 2025

Pull request type

  • Code changes (bugfix, features)
  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Tests for the changes have been added (if needed)
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally

Current behavior

#661

New behavior

We added the function to load a motor from the thrustcruve API and we tested it.

Breaking change

  • No

@Marchma0 Marchma0 requested a review from a team as a code owner November 4, 2025 18:09
@Gui-FernandesBR Gui-FernandesBR changed the base branch from master to develop November 4, 2025 18:20
@Gui-FernandesBR
Copy link
Member

I really like this implementation!!

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new method load_from_thrustcurve_api to the GenericMotor class that allows users to download motor data directly from the ThrustCurve.org API by providing a motor name. This feature simplifies motor initialization by eliminating the need to manually download .eng files.

  • Adds API integration with ThrustCurve.org to download motor data
  • Implements a new static method that searches for motors and downloads their .eng files
  • Includes comprehensive test coverage for the new functionality

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File Description
rocketpy/motors/motor.py Adds new static method load_from_thrustcurve_api with required imports (base64, tempfile, requests) to enable downloading and loading motor data from ThrustCurve API
tests/unit/motors/test_genericmotor.py Adds test case to verify the new API loading functionality works correctly with Cesaroni M1670 motor data

@Gui-FernandesBR
Copy link
Member

I see 2 major problems:

1 - Documentation: we should add a description of how to use the new feature.
2 - Tests: We should mock the API call so we don't exhaust the endpoint whenever we run the tests

@Monta120 Monta120 force-pushed the enh/motor-thrustcurve-api branch from ff57272 to 2770ba2 Compare November 4, 2025 21:32
@Monta120 Monta120 force-pushed the enh/motor-thrustcurve-api branch from 2770ba2 to da39fcb Compare November 4, 2025 21:57
@Gui-FernandesBR Gui-FernandesBR requested review from Monta120, Copilot and phmbressan and removed request for Monta120 November 5, 2025 01:34
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

@codecov
Copy link

codecov bot commented Nov 5, 2025

Codecov Report

❌ Patch coverage is 94.59459% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.31%. Comparing base (9cf3dd4) to head (d6c5dee).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
rocketpy/motors/motor.py 94.59% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #870      +/-   ##
===========================================
+ Coverage    80.27%   80.31%   +0.03%     
===========================================
  Files          104      104              
  Lines        12769    12805      +36     
===========================================
+ Hits         10250    10284      +34     
- Misses        2519     2521       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Monta120
Copy link

Monta120 commented Nov 5, 2025

@Gui-FernandesBR Thank you for going over the code. I just updated the latest commit to run make format for import/order/style cleanup and added tests for exception handling in load_from_thrustcurve_api.

@Marchma0
Copy link
Author

Hi all, We’ve updated the PR based on your feedback. Could you please check if it looks good now or if anything else needs improvement ?

@Gui-FernandesBR
Copy link
Member

Gui-FernandesBR commented Nov 12, 2025

Hi all, We’ve updated the PR based on your feedback. Could you please check if it looks good now or if anything else needs improvement ?

@Marchma0 can you check why linters are not passing? Also, if you could please mark previous comments as "resolved", that would make our review easier

@Marchma0
Copy link
Author

We updated the code, i think we resolved pylint errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

3 participants