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.

Sign up for GitHub

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

Switch to pre-commit for Git hooks #46235

Closed
wants to merge 1 commit into from
Closed

Switch to pre-commit for Git hooks #46235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .clang-tidy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,3 @@ CheckOptions:
- key: readability-braces-around-statements.ShortStatementLines
value: '0'
...

19 changes: 4 additions & 15 deletions .github/workflows/static_checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,26 +21,15 @@ jobs:
sudo apt-get install -qq dos2unix recode clang-format-11
sudo update-alternatives --remove-all clang-format
sudo update-alternatives --install /usr/bin/clang-format clang-format /usr/bin/clang-format-11 100
sudo pip3 install black==20.8b1 pygments
sudo pip3 install black==20.8b1 pygments pre-commit
pre-commit install

- name: File formatting checks (file_format.sh)
- name: Run pre-commit hooks on all files
run: |
bash ./misc/scripts/file_format.sh

- name: Style checks via clang-format (clang_format.sh)
run: |
bash ./misc/scripts/clang_format.sh

- name: Python style checks via black (black_format.sh)
run: |
bash ./misc/scripts/black_format.sh
pre-commit run --all-files

- name: JavaScript style checks via ESLint
run: |
cd platform/javascript
npm ci
npm run lint

- name: Documentation checks
run: |
doc/tools/makerst.py --dry-run doc/classes modules
43 changes: 43 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Exclude files which shouldn't be modified by pre-commit hooks.
exclude: |
(?x)^(
.*\.sln| # Expected to contain a BOM
.*\.csproj| # Expected to contain a BOM
.*\.patch| # Expected to contrain trailing whitespace
.*\.pot|
.*\.po|
thirdparty/.*| # Third-party files
platform/android/java/lib/src/com/google.*| # Third-party files
.*-so_wrap\..* # Generated code
)

repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v3.4.0
hooks:
- id: fix-byte-order-marker
- id: end-of-file-fixer
- id: trailing-whitespace

- id: mixed-line-ending
args: [--fix=lf]

- repo: https://github.com/pocc/pre-commit-hooks
rev: v1.1.1
hooks:
- id: clang-format

- repo: https://github.com/psf/black
rev: 20.8b1
hooks:
- id: black
args: [-l 120]

- repo: local
hooks:
- id: makerst
name: Check XML class reference syntax and validity
language: system
pass_filenames: false
entry: python doc/tools/makerst.py doc/classes modules --dry-run
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
entry: python doc/tools/makerst.py doc/classes modules --dry-run
entry: python3 doc/tools/makerst.py doc/classes modules --dry-run

Copy link
Member

Choose a reason for hiding this comment

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

Might need testing on Windows as typically the installed binary there is python.exe for Python 3.
And apparently @BastiaanOlij has a setup where it's py.exe 🙀

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, Windows seems to use python.exe, at least when it's installed from www.python.org. And probably won't run the script directly if python is removed from the command (which is also a valid option on macOS). I'll check whether it works on Windows at all a bit later.

Copy link
Member

Choose a reason for hiding this comment

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

I'll check whether it works on Windows at all a bit later.

Seems to work fine on Windows.

probably won't run the script directly if python is removed

Windows shell is capable of running python scripts directly, but pre-commit is not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might need testing on Windows as typically the installed binary there is python.exe for Python 3.
And apparently @BastiaanOlij has a setup where it's py.exe 🙀

Yeah but the thing is this stupid STUB thing that Windows does so if you try and run python, instead of it running your installed python, it will run this little program that redirects you to MSVC and tells it to install its version of Python. Like WTF..

py.exe worked because that didn't have a stub.

Can't remember the suggestion but someone told me how to turn that off and now the python executable in my python install works

Copy link
Contributor

Choose a reason for hiding this comment

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

pre-commit has a config option default_language_version. Setting it to python3 is enough to use Python 3 even with python executable name

minimal reproducible example:

default_language_version:
  python: python3

repos:
-   repo: local
    hooks:
    -   id: check-version
        name: check python version
        language: python
        entry: python --version
        files: ".*"

$ pre-commit run --verbose
>>> Python 3.10.6

output with python: python2:

>>> Python 2.7.17

files: ^(modules/.*/doc_classes/*.xml|doc/classes/.*.xml)$
1 change: 0 additions & 1 deletion misc/dist/ios_xcode/godot_ios/en.lproj/InfoPlist.strings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
/* Localized versions of Info.plist keys */

33 changes: 0 additions & 33 deletions misc/scripts/black_format.sh
View file
Open in desktop

This file was deleted.

58 changes: 0 additions & 58 deletions misc/scripts/clang_format.sh
View file
Open in desktop

This file was deleted.

60 changes: 0 additions & 60 deletions misc/scripts/file_format.sh
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion modules/fbx/fbx_parser/LICENSE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,3 @@ DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

1 change: 0 additions & 1 deletion modules/mono/editor/GodotTools/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,4 +353,3 @@ healthchecksdb

# Backup folder for Package Reference Convert tool in Visual Studio 2017
MigrationBackup/