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: Refactors the code to adopt pylint #621

Merged
merged 29 commits into from
Jul 5, 2024

Conversation

Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR commented Jun 14, 2024

We are going to officially adopt pylint in this repo.

Github Actions workflows erre updated:

  • The black/linter bot will no longer commit to this repo
  • When opening a PR, isort, black and pylint will be executed to verify if the code follows

I fixed many pylint errors, but it was a really hard task, quite time consuming. Therefore, I had to use pylint-silent to ignore some errors, those will be solved in the future, one by one.

You don't have to read all the files. Focus on the new github workflow files and the .pylintrc file

@Gui-FernandesBR Gui-FernandesBR marked this pull request as ready for review June 23, 2024 16:05
@Gui-FernandesBR Gui-FernandesBR requested review from a team as code owners June 23, 2024 16:05
@Gui-FernandesBR Gui-FernandesBR self-assigned this Jun 23, 2024
@Gui-FernandesBR Gui-FernandesBR added C.I. Continuous Integration (Workflows and actions) Refactor labels Jun 23, 2024
Copy link
Collaborator

@phmbressan phmbressan left a comment

Choose a reason for hiding this comment

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

Good work. I left some suggestions, I am not an expert on linters, so take those with a grain of salt: but I do think some of the defined linting rules are not strict enough.

This already has produced improvements and more idiomatic code and has potential to accelerate our reviews.

For the long term, let's keep in mind always that linters are good to suggest improvements, but they do not force you to anything, there are edge cases.

.github/workflows/linters.yml Outdated Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
rocketpy/environment/environment.py Show resolved Hide resolved
@Gui-FernandesBR
Copy link
Member Author

We should add a fallback linter: another linter that is run after pylint to capture eventual errors not captured by the main linter.

I would say that ruff, the "yougest and fastest" player in the market right now, is our best candidate. I will let it for the next PR.

Flake8 would be the more "natural" option.

Maybe we could run flake8 AND also ruff.


@phmbressan I have fixed your comments. Please take in mind that I didnt't care too much about the errors threshold at this first time. I was more worried about getting everything to run.

Copy link

codecov bot commented Jun 29, 2024

Codecov Report

Attention: Patch coverage is 78.69249% with 88 lines in your changes missing coverage. Please review.

Please upload report for BASE (develop@23fa110). Learn more about missing BASE report.

Files Patch % Lines
rocketpy/environment/environment_analysis.py 18.18% 27 Missing ⚠️
rocketpy/simulation/flight.py 58.69% 19 Missing ⚠️
rocketpy/motors/solid_motor.py 50.00% 7 Missing ⚠️
rocketpy/prints/aero_surface_prints.py 77.77% 4 Missing ⚠️
rocketpy/tools.py 42.85% 4 Missing ⚠️
rocketpy/utilities.py 75.00% 4 Missing ⚠️
rocketpy/environment/environment.py 86.95% 3 Missing ⚠️
rocketpy/plots/compare/compare.py 40.00% 3 Missing ⚠️
rocketpy/plots/rocket_plots.py 50.00% 3 Missing ⚠️
rocketpy/plots/environment_analysis_plots.py 86.66% 2 Missing ⚠️
... and 10 more
Additional details and impacted files
@@            Coverage Diff             @@
##             develop     #621   +/-   ##
==========================================
  Coverage           ?   73.91%           
==========================================
  Files              ?       70           
  Lines              ?    10032           
  Branches           ?        0           
==========================================
  Hits               ?     7415           
  Misses             ?     2617           
  Partials           ?        0           

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

.pylintrc Show resolved Hide resolved
.pylintrc Show resolved Hide resolved
rocketpy/rocket/aero_surface.py Show resolved Hide resolved
Comment on lines +878 to +880
planform_correlation_parameter = (
2 * np.pi * self.AR / (clalpha2D * np.cos(self.gamma_c))
) # pylint: disable=invalid-name
Copy link
Member

Choose a reason for hiding this comment

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

FD was used here because it is the naming used in the formulas. This makes it easier to review/understand the following equation. I don't think we gain anything from having an extensive name here

Copy link
Member Author

Choose a reason for hiding this comment

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

Speaking as someone that opens a PR every week but hasn't review any technical equation in the last few months.
I personally prefer the more readable way. I don't really know what FD means.
Making it explicit is better than implicitly.

But I understand this can be quite personal.

Still, I think you will agree with me that the fins' equations are a "hard-to-read" code block.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm reading this again.
I honestly can't agree that self.AR is more readable and less error-prune than self.aspect_ratio

Copy link
Member Author

Choose a reason for hiding this comment

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

A third opinion would be nice

rocketpy/rocket/aero_surface.py Outdated Show resolved Hide resolved
rocketpy/rocket/rocket.py Outdated Show resolved Hide resolved
rocketpy/simulation/flight.py Outdated Show resolved Hide resolved
rocketpy/simulation/flight.py Outdated Show resolved Hide resolved
rocketpy/simulation/flight.py Show resolved Hide resolved
rocketpy/simulation/flight.py Outdated Show resolved Hide resolved
@Gui-FernandesBR Gui-FernandesBR merged commit 257d16a into develop Jul 5, 2024
10 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the mnt/update-pylint-rules branch July 5, 2024 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C.I. Continuous Integration (Workflows and actions) Refactor
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants