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

Use pre-commit to invoke ruff, mypy #742

Merged
merged 7 commits into from
Oct 11, 2023
Merged

Use pre-commit to invoke ruff, mypy #742

merged 7 commits into from
Oct 11, 2023

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Sep 8, 2023

This PR streamlines the use of code quality tools:

  • Add .pre-commit-config.yaml, configuration for pre-commit to:
    • Invoke mypy.
    • Invoke black.
    • Invoke ruff.
  • Replace isort and flake8 configuration with equivalent ruff configuration.
  • Remove .github/workflow/lint.yaml with a "Code quality" job within .github/workflow/pytest.yaml that invokes pre-commit/action.

The advantages are:

  • pre-commit runs both on developers' machines and in the GitHub Actions runners, using the same configuration.
  • We no longer need to maintain iiasa/actions/.github/workflows/lint.yaml (common re-usable workflow) and .github/workflows/lint.yaml (invoking the former), and the interaction between the two.
  • The replacement "Code quality" job is significantly shorter and simpler; it also runs faster.
  • Ruff is significantly faster than flake8+isort, thus better when run via a developer's editor/IDE.

Some disadvantages:

  • The mypy configuration is a little complex:
    • In order to allow use of local development versions/editable installs for type hints, I had to write a custom "local" hook that checks an environment variable and uses a local environment.
    • We cannot (see Support updating additional_dependencies in autoupdate pre-commit/pre-commit#1351) automatically update the packages in the virtual environment that pre-commits creates for running mypy, so we have to force this in order to get early warning of upstream typing changes. This PR adds a workflow step to do this.
  • The pre-commit maintainer seems to be very brusque, so it might be hard to get help if we need it. However, our use case is simple and should remain so.

How to review

Read the diff and note that the CI checks all pass.

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • Add, expand, or update documentation.
  • Update release notes. N/A

@khaeru khaeru added enh New features & functionality ci Continuous integration labels Sep 8, 2023
@khaeru khaeru self-assigned this Sep 8, 2023
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #742 (f9afa5f) into main (31d9c58) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main    #742     +/-   ##
=======================================
- Coverage   94.3%   94.3%   -0.1%     
=======================================
  Files         43      43             
  Lines       3475    3474      -1     
=======================================
- Hits        3280    3279      -1     
  Misses       195     195             
Files Coverage Δ
message_ix/reporting/__init__.py 100.0% <ø> (ø)
message_ix/testing/__init__.py 99.6% <100.0%> (ø)

@khaeru khaeru force-pushed the enh/pre-commit branch 4 times, most recently from 29cdd58 to 1eb5c73 Compare September 8, 2023 13:03
@khaeru khaeru merged commit 6ce36a5 into main Oct 11, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration enh New features & functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant