Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MNT: making simple refactors in the Flight class #442

Merged
merged 8 commits into from
Nov 9, 2023

Conversation

Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR commented Oct 15, 2023

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 --runslow) have passed locally

Current behavior

The Flight class is one of the most complicated to be read. Regardless the sophisticated u_dot, some functions/methos were added a long time ago and needs an update.
The subclass FlightPhases is barely documented, and its error messages looks like each other too much.

New behavior

  • Improving readability and fixing some "mistakes"
  • The reason I'm adding more lines than removing is basically due the new docstrings that were missing.
  • I really hope you like the additions and find them more easy-to-read

Something important:

Breaking change

  • No

Additional information

  • Some commits have a longer description if you dropdown it.
  • I stopped making changes because the PR was getting too big already, but more improvements are still welcome.
  • The module is too large, with more than 3k code lines. Effort on increasing readability are more than necessary

- useless return drift
- use format string with interpolation
- safely opening file using 'with' context
- specify utf-8 encoding
- reduce code lines by using functions
- more pythonic for loops
- add docstring to the class and its methods
- avoid using mutable default arguments
- simple display_warning method
- add method: use early returns to increase readability
- add method: more if comprehensions, more readability
- add highlights to warning messages
@Gui-FernandesBR Gui-FernandesBR added Docs Docs and examples related Refactor Flight Flight Class related features labels Oct 15, 2023
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Oct 15, 2023
@Gui-FernandesBR Gui-FernandesBR self-assigned this Oct 15, 2023
rocketpy/simulation/flight.py Outdated Show resolved Hide resolved
@Gui-FernandesBR
Copy link
Member Author

Copying the commentary here in case we need it in the future:

Ok, this is actually the best PEP to understand about the problem: https://peps.python.org/pep-0597/

Summarizing... It actually does not matter that much in this case since we are just saving a text file with numbers separated by comma. Any encoding should work here.

However, not specifying the encoding when using open() is a bad practice since PEP0597. The 'utf-8' is standard in Python and is usually more flexible than ascii.

To be "platform-dependent", i.e. to write the files using the same encoding as the local machine, it's possible to use encoding="locale", but this only works on python 3.10 and above. See this section for a short description: https://peps.python.org/pep-0597/#for-experienced-users

All in all, this is not a problem for now. I'm expecting that one day in the future we can revisit this issue in the long term future when Python start to enforce the explicit encoding definition everywhere.

@Gui-FernandesBR Gui-FernandesBR merged commit 2352f0c into develop Nov 9, 2023
11 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the mnt/simple-refactors-flight branch November 9, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs Docs and examples related Flight Flight Class related features Refactor
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

2 participants