Skip to content

Ruff newrules UP RET FBT FA PTH#609

Merged
enricostragiotti merged 6 commits intoruff_newrulesfrom
ruff_newrules_UP_RET_FBT_FA_PTH
Apr 17, 2025
Merged

Ruff newrules UP RET FBT FA PTH#609
enricostragiotti merged 6 commits intoruff_newrulesfrom
ruff_newrules_UP_RET_FBT_FA_PTH

Conversation

@enricostragiotti
Copy link
Copy Markdown
Contributor

@enricostragiotti enricostragiotti commented Mar 11, 2025

Summary of Changes

This is the first PR introducing Ruff-based linting rules to the project to solve #610 . Currently, we rely on Codacy to enforce code quality by preventing merges when issues are detected. However, Codacy only runs checks online, requiring developers to push their code and wait for feedback before making adjustments.

The goal of this PR is to integrate Ruff checks into our pre-commit hooks, allowing developers to check their code locally before pushing. This avoids unnecessary uploads, reduces feedback delays, and ensures that developers cannot bypass rules simply by configuring Codacy to ignore them. Instead, any ignored rule must be explicitly marked with a noqa directive in the code, along with a comment explaining the reason, or disabled globally in the Ruff configuration.

Introduced Ruff Rules

This first batch enforces the following Ruff rules, which we agreed are important:

  • UP (pyupgrade): Ensures compatibility with modern Python versions by replacing outdated syntax. Documentation or orignial repo
  • RET (return): Enforces best practices for return statements. Documentation
  • FBT (flake8-boolean-trap): Prevents common issues with boolean function arguments. Documentation
  • FA (flake8-annotations): Ensures function annotations are properly used. Documentation
  • PTH (flake8-use-pathlib): Encourages the use of pathlib over os.path. Documentation

Additional Changes

  • Separated Ruff configuration:
    The Ruff configuration (ruff.toml) has been moved out of pyproject.toml to keep things clean and readable, as the configuration is expected to grow over time.

  • Ensuring the correct Ruff version in pre-commit:
    The pre-commit hook now runs Ruff using Poetry to ensure the correct project-specific version is used, rather than relying on a globally installed version. This prevents potential issues, particularly in VS Code.

  • Modification of _is_shape_by_conn to comply with new typing rules:
    The function _is_shape_by_conn(key, segment_class) -> bool: has been modified in compliance with the new typing rules enforced by Ruff (especially problems with ClassVar and lazy type evaluation introduced by using builtin types).

New Dependencies

updated ruff to 0.9.9

Checklist

  • Have you followed the guidelines in our Contributing wiki?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added new tests to cover your changes?
  • Have you made corresponding changes to the documentation? If yes, please consider providing a link to the updated documentation.
  • Do the existing tests pass with your changes?
  • Have you commented on hard-to-understand areas of the code?
  • Does this PR introduce a breaking change?

Additional Context

First batch of new rules, next batch is coming once these modifications are accepted.

@enricostragiotti enricostragiotti added the development Related to development process and configuration label Mar 11, 2025
@enricostragiotti enricostragiotti self-assigned this Mar 11, 2025
@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Copy Markdown
Contributor

@esnguyenvan esnguyenvan left a comment

Choose a reason for hiding this comment

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

Hey, thanks for this important work. Overall I agree with the changes, it makes the code nice and consistent. I have a few questions and remarks as well.

@esnguyenvan
Copy link
Copy Markdown
Contributor

In addition to the previous comments, the changes brought by this PR will make the following integration tests of CS25 break:

  • FAILED tests/integration_tests/oad_process/test_oad_process.py::test_api_eval_breguet - AttributeError: 'bool' object has no attribute 'lower'
  • FAILED tests/integration_tests/oad_process/test_oad_process.py::test_api_eval_mission - AttributeError: 'bool' object has no attribute 'lower'
  • FAILED tests/integration_tests/oad_process/test_oad_process.py::test_api_optim - AttributeError: 'bool' object has no attribute 'lower'

@enricostragiotti
Copy link
Copy Markdown
Contributor Author

In addition to the previous comments, the changes brought by this PR will make the following integration tests of CS25 break:

* FAILED tests/integration_tests/oad_process/test_oad_process.py::test_api_eval_breguet - AttributeError: 'bool' object has no attribute 'lower'

* FAILED tests/integration_tests/oad_process/test_oad_process.py::test_api_eval_mission - AttributeError: 'bool' object has no attribute 'lower'

* FAILED tests/integration_tests/oad_process/test_oad_process.py::test_api_optim - AttributeError: 'bool' object has no attribute 'lower'

That's annoying. Thanks for the spotting! I'll look into that!

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 2, 2025

Codecov Report

Attention: Patch coverage is 96.32546% with 14 lines in your changes missing coverage. Please review.

Project coverage is 91.91%. Comparing base (f446c6d) to head (9d4b868).
Report is 7 commits behind head on ruff_newrules.

Files with missing lines Patch % Lines
...oad/io/xml/resources/remove_duplicate_variables.py 0.00% 4 Missing ⚠️
src/fastoad/_utils/strings.py 0.00% 2 Missing ⚠️
src/fastoad/gui/optimization_viewer.py 81.81% 2 Missing ⚠️
src/fastoad/_utils/files.py 75.00% 0 Missing and 1 partial ⚠️
src/fastoad/exceptions.py 0.00% 1 Missing ⚠️
src/fastoad/gui/variable_viewer.py 91.66% 1 Missing ⚠️
src/fastoad/models/performances/mission/base.py 88.88% 1 Missing ⚠️
src/fastoad/models/performances/mission/mission.py 88.88% 0 Missing and 1 partial ⚠️
src/fastoad/openmdao/variables/variable_list.py 91.66% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           ruff_newrules     #609   +/-   ##
==============================================
  Coverage          91.91%   91.91%           
==============================================
  Files                100       99    -1     
  Lines               6184     6188    +4     
  Branches             979      979           
==============================================
+ Hits                5684     5688    +4     
+ Misses               334      333    -1     
- Partials             166      167    +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@enricostragiotti
Copy link
Copy Markdown
Contributor Author

thanks @esnguyenvan for the remarks ! I completely reworked the logic of the _is_shape_by_conn method, just by handling in a try except the exception that we had because of the use of from future import annotations and the lazy type evaluation.
I also checked that now all the CS25 tests pass. Could you tell me when you do it if the RTA tests pass as well? Thank you very much !

Copy link
Copy Markdown
Contributor

@esnguyenvan esnguyenvan left a comment

Choose a reason for hiding this comment

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

Thanks for the rework, it's quite nice! It's all good for me here. It's all good with the RTA repo

@enricostragiotti
Copy link
Copy Markdown
Contributor Author

Perfect! Let's wait for @ScottDelbecq review then! thanks Éric!

@enricostragiotti enricostragiotti merged commit 4e7d601 into ruff_newrules Apr 17, 2025
14 checks passed
@enricostragiotti enricostragiotti deleted the ruff_newrules_UP_RET_FBT_FA_PTH branch April 17, 2025 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

development Related to development process and configuration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants