MNT: Refactors the code to adopt pylint#621
Conversation
Gui-FernandesBR
added
C.I.
phmbressan
left a comment
phmbressan
left a comment
There was a problem hiding this comment.
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.
|
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 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. |
Codecov ReportAttention: Patch coverage is
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. |
| planform_correlation_parameter = ( | ||
| 2 * np.pi * self.AR / (clalpha2D * np.cos(self.gamma_c)) | ||
| ) # pylint: disable=invalid-name |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm reading this again.
I honestly can't agree that self.AR is more readable and less error-prune than self.aspect_ratio
There was a problem hiding this comment.
A third opinion would be nice
3 participants
We are going to officially adopt pylint in this repo.
Github Actions workflows erre updated:
I fixed
manypylint 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