Skip to content

Conversation

CameronBrooks11
Copy link

@CameronBrooks11 CameronBrooks11 commented Aug 27, 2025

Pull request type

  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

Export routines for Flight live as instance methods in rocketpy/simulation/flight.py (export_data, export_pressures, export_sensor_data, export_kml). This mixes simulation with I/O concerns and forces an import-time dependency on simplekml for users who do not export.

New behavior

  • Extracted exporters into a new class FlightDataExporter at rocketpy/simulation/flight_data_exporter.py.
  • Added deprecation wrappers on Flight methods that delegate to FlightDataExporter and emit DeprecationWarning with migration guidance.
  • Re-exported FlightDataExporter in rocketpy/simulation/__init__.py.
  • No functional changes to outputs or signatures aside from deprecation.
  • Deprecation schedule: wrappers remain until v1.12.0.
  • Migration example:
from rocketpy.simulation import FlightDataExporter

exporter = FlightDataExporter(test_flight)
exporter.export_data("out.csv", "angle_of_attack", "mach_number")

Breaking change

  • No

Additional information

  • Local runs show deprecation warnings during integration tests where legacy methods are exercised. Example: Moved to FlightDataExporter.export_data() and will be removed in v1.12.0.
  • Headless plotting for integration tests was configured with MPLBACKEND=Agg.
  • Formatting-only changes were intentionally excluded from this PR to keep the diff focused on the refactor.

@Gui-FernandesBR
Copy link
Member

I really like what I see here... I will take a look at it asap

Copy link

codecov bot commented Aug 27, 2025

Codecov Report

❌ Patch coverage is 81.81818% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.28%. Comparing base (f17893b) to head (b88abc4).
⚠️ Report is 25 commits behind head on develop.

Files with missing lines Patch % Lines
rocketpy/simulation/flight_data_exporter.py 79.31% 18 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #845      +/-   ##
===========================================
+ Coverage    80.02%   80.28%   +0.26%     
===========================================
  Files           98      104       +6     
  Lines        12004    12754     +750     
===========================================
+ Hits          9606    10240     +634     
- Misses        2398     2514     +116     

☔ 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.

@CameronBrooks11
Copy link
Author

Sounds good, let me know if there's anything that needs to be done or revised before this can be merged to develop...I have some enhancements of the kml export I have in the works and I'd rather not stack the PR branches and have to deal with rebasing later if we can help it :)

@CameronBrooks11
Copy link
Author

@Gui-FernandesBR Sorry if this PR disappeared for a few days...my account got false flagged by the GitHub bots

@CameronBrooks11
Copy link
Author

Hi, it's been over a month now and still waiting on a reviewer to be assigned and take a look. I actually spent extra effort trying to make the PR extra nice and clean so it could just get merged quickly...like I said I had intended to continue more PRs concerning file IO but this going stale has kinda thrown a wrench in that.

Copy link

@Copilot 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 extracts flight data export functionality from the Flight class into a dedicated FlightDataExporter class to improve separation of concerns and reduce import-time dependencies.

  • Moved four export methods (export_data, export_pressures, export_sensor_data, export_kml) from Flight to FlightDataExporter
  • Added deprecation wrappers in the Flight class that delegate to the new exporter
  • Updated documentation to showcase the new API with migration examples

Reviewed Changes

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

Show a summary per file
File Description
rocketpy/simulation/flight_data_exporter.py New class containing extracted export functionality
rocketpy/simulation/flight.py Deprecated export methods now delegate to FlightDataExporter
rocketpy/simulation/__init__.py Re-exports FlightDataExporter for public API access
tests/unit/test_flight_export_deprecation.py Tests verifying deprecation warnings and delegation behavior
tests/unit/test_flight_data_exporter.py Tests for the new exporter class functionality
docs/user/first_simulation.rst Updated examples to use new API with deprecation notes
CHANGELOG.md Documents the deprecation and extraction changes

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

Brilliant work!!

@CameronBrooks11 I will just wait a few more days so either @phmbressan or @MateusStano give us a second approve and then we will proceed to merge.

@github-project-automation github-project-automation bot moved this from Backlog to Next Version in LibDev Roadmap Oct 4, 2025
@Gui-FernandesBR Gui-FernandesBR force-pushed the mnt/extract-flight-data-exporters branch from a03cca8 to a7edc8e Compare October 5, 2025 00:02
@Gui-FernandesBR Gui-FernandesBR force-pushed the mnt/extract-flight-data-exporters branch from a7edc8e to b88abc4 Compare October 5, 2025 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Next Version
Development

Successfully merging this pull request may close these issues.

2 participants