-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Replace hand-written pre-commit hooks with pre-commit
Python tool
#88866
Conversation
f43f8b8
to
d1f9542
Compare
Isn't there some missing scripts in the new |
|
`pre-commit` can be installed with pip, and configured in the Godot repo with `pre-commit install`. It can then easily be run both locally with `pre-commit run`, and on CI, in a cross-platform way. This makes it much easier for contributors to set up pre-commit hooks, without having to manually copy files to their git folder. Co-authored-by: Rémi Verschelde <[email protected]>
d1f9542
to
20c563d
Compare
For the record / future work, I looked into moving diff --git a/.github/workflows/static_checks.yml b/.github/workflows/static_checks.yml
index a9808fee95..09a75b481e 100644
--- a/.github/workflows/static_checks.yml
+++ b/.github/workflows/static_checks.yml
@@ -23,7 +23,7 @@ jobs:
- name: Install Python dependencies and general setup
run: |
- pip3 install pytest==7.1.2 mypy==0.971
+ pip3 install pytest==7.1.2
git config diff.wsErrorHighlight all
- name: Get changed files
@@ -57,14 +57,6 @@ jobs:
run: |
bash ./misc/scripts/header_guards.sh changed.txt
- - name: Python scripts static analysis (mypy_check.sh)
- run: |
- if grep -qE '\.py$|SConstruct|SCsub' changed.txt || [ -z "$(cat changed.txt)" ]; then
- bash ./misc/scripts/mypy_check.sh
- else
- echo "Skipping Python static analysis as no Python files were changed."
- fi
-
- name: Python builders checks via pytest (pytest_builders.sh)
run: |
bash ./misc/scripts/pytest_builders.sh
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 3493219ea7..d30e4075cf 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -23,6 +23,15 @@ repos:
args:
- --line-length=120
+ - repo: https://github.com/pre-commit/mirrors-mypy
+ rev: v0.971
+ hooks:
+ - id: mypy
+ files: (\.py$|SConstruct|SCsub)
+ types_or: [text]
+ exclude: .*thirdparty.*
+ args: [--config-file=./misc/scripts/mypy.ini]
+
- repo: local
hooks:
- id: make-rst
diff --git a/misc/scripts/mypy_check.sh b/misc/scripts/mypy_check.sh
deleted file mode 100755
index 2a06486d67..0000000000
--- a/misc/scripts/mypy_check.sh
+++ /dev/null
@@ -1,6 +0,0 @@
-#!/usr/bin/env bash
-
-set -uo pipefail
-
-echo -e "Python: mypy static analysis..."
-mypy --config-file=./misc/scripts/mypy.ini . |
Sounds good, I'll do that in a future PR 🙂 Edit: Pull request opened: #89233 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this!
How to test
Do some changes that would fail tests, and try to commit, or run
pre-commit run
manually.pre-commit
can be installed with pip, and configured in the Godot repo withpre-commit install
. It can then easily be run both locally withpre-commit run
, and on CI, in a cross-platform way.This makes it much easier for contributors to set up pre-commit hooks, without having to manually copy files to their git folder.
pre-commit
for pre-commit hooks and CI #87003I've included fixes for the issues I mentioned in #87003 (comment):
black
ids back together and made it apply on all.py
,SConstruct
andSCsub
files, instead of relying ontypes_or: [python]
which doesn't know SCons files.pre-commit
hook at https://github.com/pre-commit/mirrors-clang-format, which takes care of installing the clang-format version we want via Python. This solves the need of ensuring everyone uses the same clang-format version installed locally.misc/hooks/
files which are no longer needed.misc/scripts/{black,clang}_format.sh
, replaced by runningpre-commit run
locally.@Calinou IMO some of the changes from #46235 may still be worth bringing over, maybe as a follow up PR. You seemed to have found pre-existing hooks that can help us get rid of
misc/scripts/file_format.sh
, and the global exclude pattern may not be a bad idea.We can likely look into moving more of the
misc/scripts/
/ CI checks topre-commit
, e.g. dotnet format (CC @godotengine/dotnet) or codespell.