Skip to content

Conversation

@Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR commented Nov 28, 2023

Pull request type

  • Code changes (bugfix, features)
  • Code maintenance (refactoring, formatting, tests)
  • ReadMe, Docs and GitHub updates
  • Other (please describe):

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 --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

Enter text here...

New behavior

This is useful for debugging or when you simply call Rocket.parachutes and want to see something more readable.

Breaking change

  • Yes
  • No

Additional information

Opening this now, can describe the PR later, sorry.

@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Nov 28, 2023
@Gui-FernandesBR Gui-FernandesBR added the Parachute Related to parachutes methods and usage label Nov 28, 2023
@codecov
Copy link

codecov bot commented Nov 28, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (a6dfea3) 71.05% compared to head (f4437d3) 70.91%.
Report is 11 commits behind head on develop.

❗ Current head f4437d3 differs from pull request most recent head 8def7ad. Consider uploading reports for the commit 8def7ad to get more accurate results

Files Patch % Lines
rocketpy/rocket/parachute.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #490      +/-   ##
===========================================
- Coverage    71.05%   70.91%   -0.14%     
===========================================
  Files           55       55              
  Lines         9261     9265       +4     
===========================================
- Hits          6580     6570      -10     
- Misses        2681     2695      +14     
Flag Coverage Δ
unittests 70.91% <50.00%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@giovaniceotto giovaniceotto left a comment

Choose a reason for hiding this comment

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

@Gui-FernandesBR, I understand the usefullness of these codelines. However, I believe it would be a good practice to follow Python conventions for the __repr__ method, specially since this is mostly for dev use.

From https://docs.python.org/3/reference/datamodel.html#object.repr:

If at all possible, this should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment). If this is not possible, a string of the form <...some useful description...> should be returned.

Options are:

f"<Parachute(name={self.name}, cd_s={self.cd_s}, trigger={self.trigger}, sampling_rate={self.sampling_rate}, lag={self.lag}, noise={self.noise})>"

or, less verbose,

f"<Parachute name={self.name} (cd_s = {self.cd_s:.4f} m2, trigger = {self.trigger})>"

Look at https://stackoverflow.com/questions/44595218/python-repr-for-all-member-variables for more inspiration.

Base automatically changed from enh/case-sensitive-triggers to develop November 30, 2023 05:23
@Gui-FernandesBR Gui-FernandesBR requested a review from a team as a code owner November 30, 2023 05:23
@Gui-FernandesBR Gui-FernandesBR merged commit a9ac31c into develop Nov 30, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the mnt/parachute-repr-method branch November 30, 2023 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Parachute Related to parachutes methods and usage

Projects

No open projects
Status: Closed

Development

Successfully merging this pull request may close these issues.

3 participants