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

DOCs: linter corrections to Function class #446

Merged
merged 13 commits into from
Nov 16, 2023

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest --runslow) have passed locally

Current behavior

  • The Function class is hard to read: code is mostly dated from 4 years ago, some list operations takes several code lines
  • The black linter doesn't cover all the problems reported by pylint, which makes my IDE throw several errors and fix suggestions.
  • Some docstrings are missing
  • Some variables still use camelCase

New behavior

  • Small changes, no break changes, less errors, more readability!
  • Overall pylint score in the function.py file:
    • before: 7.25/10
    • after: 9.55/10
  • Also, added .flake8 and .pylintrc files to set the configuration for our project. Ideally, this should be
  • For the future: introduce isort and mypy integration.
  • All of these is improving the general quality of our code source, putting us in a better position as a python library

Breaking change

  • No

Additional information

  • There is still a major problem in this file which is the too generic exception inside the funcify decorator, as recently pointed out by @phmbressan I left a TODO for the future.
  • Please notice the diff shows 800 added lines, but more 600 are just the new .pylintrc file, which is generated automatically and doesn't need to be reviewed. So this PR essencially introduces 200 new code lines.
  • Waiting for review if the action get checked

- improve docstrings
- remove useless comments
- convert a few variables to snake_case to improve readability
- better exception capture
- pylint C0200 - use enumerate instead of range(len(x))
- pylint W0201 - attribute-defined-outside-init
- pylint W0102 - dangerous-default-value
- pylint C3001 - unnecessary-lambda-assignment
- pylint W1514 - better to specify an encoding when opening file
- pylint R1719 - simplifiable-if-expression
- improve more variables names
- no more else after break statement
@Gui-FernandesBR Gui-FernandesBR added the Docs Docs and examples related label Oct 29, 2023
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Oct 29, 2023
@Gui-FernandesBR Gui-FernandesBR self-assigned this Oct 29, 2023
@github-actions github-actions bot requested a review from phmbressan October 29, 2023 03:18
@Gui-FernandesBR
Copy link
Member Author

Hey, I'd like to discuss this suggestion:

image

@Gui-FernandesBR
Copy link
Member Author

Should we add a page in the documentation describing how to use the linters during the development phases?

rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Show resolved Hide resolved
.flake8 Outdated Show resolved Hide resolved
@MateusStano
Copy link
Member

Hey, I'd like to discuss this suggestion:

I prefer plot1D over plot1d, in fact I think that's why I did not change it in the snake case PR. But I get that it technically is not snake case

If plot1d is the best way forward then I think the suggestion is the best way to go

@Gui-FernandesBR
Copy link
Member Author

Hey, I'd like to discuss this suggestion:

I prefer plot1D over plot1d, in fact I think that's why I did not change it in the snake case PR. But I get that it technically is not snake case

If plot1d is the best way forward then I think the suggestion is the best way to go

I understand that you prefer one option above other. But I think we should try to be as intuitive as possible with the methods names. If rocketpy follows snake_case, then it is easier to imagine that te function is called plot1d than plot1D. Imagine the case where you have to spell the function name to someone else, that would be confusing I guess. Also, I cannot see any advantage for using the plot1D name besides of your preference, can you?

Alternatively, we could also write plot1d = plot1D in any part of this file so both options are accepted, but this is a sort of a workaround.

rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
.pylintrc Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
rocketpy/mathutils/function.py Outdated Show resolved Hide resolved
- Update Python version in the .pylintrc file
- fix variable initialization comment
- changes matrix to sys_coeffs
@Gui-FernandesBR
Copy link
Member Author

All good now. Just waiting for @MateusStano 's feedback regarding my recent changes and further discussions on the plot2D snake case situation.

@Gui-FernandesBR
Copy link
Member Author

As discussed, we are going adopt the approach of raising deprecation and renaming the function to snake case.

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (develop@53176f6). Click here to learn what that means.

Additional details and impacted files
@@            Coverage Diff             @@
##             develop     #446   +/-   ##
==========================================
  Coverage           ?   68.92%           
==========================================
  Files              ?       55           
  Lines              ?     8961           
  Branches           ?        0           
==========================================
  Hits               ?     6176           
  Misses             ?     2785           
  Partials           ?        0           
Flag Coverage Δ
unittests 68.92% <0.00%> (?)

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.

@Gui-FernandesBR
Copy link
Member Author

The --runslow option is running perfectly here too.

@Gui-FernandesBR Gui-FernandesBR merged commit f2be825 into develop Nov 16, 2023
11 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the docs/linter_corrections branch November 16, 2023 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs Docs and examples related
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

4 participants