From cd4e46ee65dab6baa6a143bf3b3f64244be36712 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Verschelde?= Date: Mon, 30 Mar 2020 08:28:32 +0200 Subject: SCons: Format buildsystem files with psf/black Configured for a max line length of 120 characters. psf/black is very opinionated and purposely doesn't leave much room for configuration. The output is mostly OK so that should be fine for us, but some things worth noting: - Manually wrapped strings will be reflowed, so by using a line length of 120 for the sake of preserving readability for our long command calls, it also means that some manually wrapped strings are back on the same line and should be manually merged again. - Code generators using string concatenation extensively look awful, since black puts each operand on a single line. We need to refactor these generators to use more pythonic string formatting, for which many options are available (`%`, `format` or f-strings). - CI checks and a pre-commit hook will be added to ensure that future buildsystem changes are well-formatted. --- misc/scripts/fix_headers.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) (limited to 'misc') diff --git a/misc/scripts/fix_headers.py b/misc/scripts/fix_headers.py index f0038a8351..7af97eec4b 100755 --- a/misc/scripts/fix_headers.py +++ b/misc/scripts/fix_headers.py @@ -37,24 +37,24 @@ files = open("files", "r") fname = files.readline() -while (fname != ""): +while fname != "": # Handle replacing $filename with actual filename and keep alignment fsingle = fname.strip() - if (fsingle.find("/") != -1): - fsingle = fsingle[fsingle.rfind("/") + 1:] + if fsingle.find("/") != -1: + fsingle = fsingle[fsingle.rfind("/") + 1 :] rep_fl = "$filename" rep_fi = fsingle len_fl = len(rep_fl) len_fi = len(rep_fi) # Pad with spaces to keep alignment - if (len_fi < len_fl): + if len_fi < len_fl: for x in range(len_fl - len_fi): rep_fi += " " - elif (len_fl < len_fi): + elif len_fl < len_fi: for x in range(len_fi - len_fl): rep_fl += " " - if (header.find(rep_fl) != -1): + if header.find(rep_fl) != -1: text = header.replace(rep_fl, rep_fi) else: text = header.replace("$filename", fsingle) @@ -71,21 +71,21 @@ while (fname != ""): line = fileread.readline() header_done = False - while (line.strip() == ""): # Skip empty lines at the top + while line.strip() == "": # Skip empty lines at the top line = fileread.readline() - if (line.find("/**********") == -1): # Godot header starts this way + if line.find("/**********") == -1: # Godot header starts this way # Maybe starting with a non-Godot comment, abort header magic header_done = True - while (not header_done): # Handle header now - if (line.find("/*") != 0): # No more starting with a comment + while not header_done: # Handle header now + if line.find("/*") != 0: # No more starting with a comment header_done = True - if (line.strip() != ""): + if line.strip() != "": text += line line = fileread.readline() - while (line != ""): # Dump everything until EOF + while line != "": # Dump everything until EOF text += line line = fileread.readline() -- cgit v1.2.3 From 164826a39bca2fb7b7277752cbc1df8833ce0f1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Verschelde?= Date: Mon, 30 Mar 2020 08:55:21 +0200 Subject: Hooks: Add pre-commit hook for psf/black formatting --- misc/hooks/README.md | 26 +++++--- misc/hooks/pre-commit | 2 +- misc/hooks/pre-commit-black | 128 +++++++++++++++++++++++++++++++++++++ misc/hooks/pre-commit-clang-format | 6 +- 4 files changed, 146 insertions(+), 16 deletions(-) create mode 100755 misc/hooks/pre-commit-black (limited to 'misc') diff --git a/misc/hooks/README.md b/misc/hooks/README.md index b18ba7df38..dad5300a09 100644 --- a/misc/hooks/README.md +++ b/misc/hooks/README.md @@ -5,16 +5,22 @@ contributors to make sure they comply with our requirements. ## List of hooks -- Pre-commit hook for clang-format: Applies clang-format to the staged files - before accepting a commit; blocks the commit and generates a patch if the - style is not respected. - Should work on Linux and macOS. You may need to edit the file if your - clang-format binary is not in the `$PATH`, or if you want to enable colored - output with pygmentize. -- Pre-commit hook for makerst: Checks the class reference syntax using `makerst.py`. - Should work on Linux and macOS. +- Pre-commit hook for `clang-format`: Applies `clang-format` to the staged + files before accepting a commit; blocks the commit and generates a patch if + the style is not respected. + You may need to edit the file if your `clang-format` binary is not in the + `PATH`, or if you want to enable colored output with `pygmentize`. +- Pre-commit hook for `black`: Applies `black` to the staged Python files + before accepting a commit. +- Pre-commit hook for `makerst`: Checks the class reference syntax using + `makerst.py`. ## Installation -Copy all the files from this folder into your `.git/hooks` folder, and make sure -the hooks and helper scripts are executable. +Copy all the files from this folder into your `.git/hooks` folder, and make +sure the hooks and helper scripts are executable. + +The hooks rely on bash scripts and tools which should be in the system `PATH`, +so they should work out of the box on Linux/macOS, and might work on Windows +when using `git-bash.exe` with `clang-format`, Python, `black`, etc. in the +`PATH`. diff --git a/misc/hooks/pre-commit b/misc/hooks/pre-commit index 36e9935785..40cb00253b 100755 --- a/misc/hooks/pre-commit +++ b/misc/hooks/pre-commit @@ -14,7 +14,7 @@ # as this script. Hooks should return 0 if successful and nonzero to cancel the # commit. They are executed in the order in which they are listed. #HOOKS="pre-commit-compile pre-commit-uncrustify" -HOOKS="pre-commit-clang-format pre-commit-makerst" +HOOKS="pre-commit-clang-format pre-commit-black pre-commit-makerst" ########################################################### # There should be no need to change anything below this line. diff --git a/misc/hooks/pre-commit-black b/misc/hooks/pre-commit-black new file mode 100755 index 0000000000..3dd0a13330 --- /dev/null +++ b/misc/hooks/pre-commit-black @@ -0,0 +1,128 @@ +#!/usr/bin/env bash + +# git pre-commit hook that runs a black stylecheck. +# Based on pre-commit-clang-format. + +################################################################## +# SETTINGS +# Set path to black binary. +BLACK=`which black` +BLACK_OPTIONS="-l 120" + +# Remove any older patches from previous commits. Set to true or false. +DELETE_OLD_PATCHES=false + +# File types to parse. +FILE_NAMES="SConstruct SCsub" +FILE_EXTS="py" + +# Use pygmentize instead of cat to parse diff with highlighting. +# Install it with `pip install pygments` (Linux) or `easy_install Pygments` (Mac) +# READER="pygmentize -l diff" +READER=cat + +################################################################## +# There should be no need to change anything below this line. + +. "$(dirname -- "$0")/canonicalize_filename.sh" + +# exit on error +set -e + +# check whether the given file matches any of the set extensions +matches_name_or_extension() { + local filename=$(basename "$1") + local extension=".${filename##*.}" + + for name in $FILE_NAMES; do [[ "$name" == "$filename" ]] && return 0; done + for ext in $FILE_EXTS; do [[ "$ext" == "$extension" ]] && return 0; done + + return 1 +} + +# necessary check for initial commit +if git rev-parse --verify HEAD >/dev/null 2>&1 ; then + against=HEAD +else + # Initial commit: diff against an empty tree object + against=4b825dc642cb6eb9a060e54bf8d69288fbee4904 +fi + +if [ ! -x "$BLACK" ] ; then + printf "Error: black executable not found.\n" + printf "Set the correct path in $(canonicalize_filename "$0").\n" + exit 1 +fi + +# create a random filename to store our generated patch +prefix="pre-commit-black" +suffix="$(date +%s)" +patch="/tmp/$prefix-$suffix.patch" + +# clean up any older black patches +$DELETE_OLD_PATCHES && rm -f /tmp/$prefix*.patch + +# create one patch containing all changes to the files +git diff-index --cached --diff-filter=ACMR --name-only $against -- | while read file; +do + # ignore thirdparty files + if grep -q "thirdparty" <<< $file; then + continue; + fi + + # ignore file if not one of the names or extensions we handle + if ! matches_name_or_extension "$file"; then + continue; + fi + + # format our file with black, create a patch with diff and append it to our $patch + # The sed call is necessary to transform the patch from + # --- $file timestamp + # +++ $file timestamp + # to both lines working on the same file and having a/ and b/ prefix. + # Else it can not be applied with 'git apply'. + "$BLACK" "$BLACK_OPTIONS" --diff "$file" | \ + sed -e "1s|--- |--- a/|" -e "2s|+++ |+++ b/|" >> "$patch" +done + +# if no patch has been generated all is ok, clean up the file stub and exit +if [ ! -s "$patch" ] ; then + printf "Files in this commit comply with the black formatter rules.\n" + rm -f "$patch" + exit 0 +fi + +# a patch has been created, notify the user and exit +printf "\nThe following differences were found between the code to commit " +printf "and the black formatter rules:\n\n" +$READER "$patch" +printf "\n" + +# Allows us to read user input below, assigns stdin to keyboard +exec < /dev/tty + +while true; do + read -p "Do you want to apply that patch (Y - Apply, N - Do not apply, S - Apply and stage files)? [Y/N/S] " yn + case $yn in + [Yy] ) git apply $patch; + printf "The patch was applied. You can now stage the changes and commit again.\n\n"; + break + ;; + [Nn] ) printf "\nYou can apply these changes with:\n git apply $patch\n"; + printf "(may need to be called from the root directory of your repository)\n"; + printf "Aborting commit. Apply changes and commit again or skip checking with"; + printf " --no-verify (not recommended).\n\n"; + break + ;; + [Ss] ) git apply $patch; + git diff-index --cached --diff-filter=ACMR --name-only $against -- | while read file; + do git add $file; + done + printf "The patch was applied and the changed files staged. You can now commit.\n\n"; + break + ;; + * ) echo "Please answer yes or no." + ;; + esac +done +exit 1 # we don't commit in any case diff --git a/misc/hooks/pre-commit-clang-format b/misc/hooks/pre-commit-clang-format index e309233a8b..f3689890df 100755 --- a/misc/hooks/pre-commit-clang-format +++ b/misc/hooks/pre-commit-clang-format @@ -15,22 +15,18 @@ ################################################################## # SETTINGS -# Set path to clang-format binary -# CLANG_FORMAT="/usr/bin/clang-format" +# Set path to clang-format binary. CLANG_FORMAT=`which clang-format` # Remove any older patches from previous commits. Set to true or false. -# DELETE_OLD_PATCHES=false DELETE_OLD_PATCHES=false # Only parse files with the extensions in FILE_EXTS. Set to true or false. # If false every changed file in the commit will be parsed with clang-format. # If true only files matching one of the extensions are parsed with clang-format. -# PARSE_EXTS=true PARSE_EXTS=true # File types to parse. Only effective when PARSE_EXTS is true. -# FILE_EXTS=".c .h .cpp .hpp" FILE_EXTS=".c .h .cpp .hpp .cc .hh .cxx .m .mm .inc .java .glsl" # Use pygmentize instead of cat to parse diff with highlighting. -- cgit v1.2.3 From 4d52761da6f15ee0374a4cac958cc7cd12507adc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Verschelde?= Date: Mon, 30 Mar 2020 09:03:38 +0200 Subject: Hooks: Use pygmentize if available to visualize diff --- misc/hooks/pre-commit-black | 8 ++++++-- misc/hooks/pre-commit-clang-format | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) (limited to 'misc') diff --git a/misc/hooks/pre-commit-black b/misc/hooks/pre-commit-black index 3dd0a13330..2dcc2e8cf1 100755 --- a/misc/hooks/pre-commit-black +++ b/misc/hooks/pre-commit-black @@ -18,8 +18,12 @@ FILE_EXTS="py" # Use pygmentize instead of cat to parse diff with highlighting. # Install it with `pip install pygments` (Linux) or `easy_install Pygments` (Mac) -# READER="pygmentize -l diff" -READER=cat +PYGMENTIZE=`which pygmentize` +if [ ! -z "$PYGMENTIZE" ]; then + READER="pygmentize -l diff" +else + READER=cat +fi ################################################################## # There should be no need to change anything below this line. diff --git a/misc/hooks/pre-commit-clang-format b/misc/hooks/pre-commit-clang-format index f3689890df..c5cf4ecbb1 100755 --- a/misc/hooks/pre-commit-clang-format +++ b/misc/hooks/pre-commit-clang-format @@ -31,8 +31,12 @@ FILE_EXTS=".c .h .cpp .hpp .cc .hh .cxx .m .mm .inc .java .glsl" # Use pygmentize instead of cat to parse diff with highlighting. # Install it with `pip install pygments` (Linux) or `easy_install Pygments` (Mac) -# READER="pygmentize -l diff" -READER=cat +PYGMENTIZE=`which pygmentize` +if [ ! -z "$PYGMENTIZE" ]; then + READER="pygmentize -l diff" +else + READER=cat +fi ################################################################## # There should be no need to change anything below this line. -- cgit v1.2.3 From 3644036fd3d8678ac44695cc49ae63c4aaeb1b97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Verschelde?= Date: Mon, 30 Mar 2020 09:10:37 +0200 Subject: Travis: Add static check for Python black formatting Also install and use pygmentize to visualize clang-format and black diffs. --- misc/travis/black-format.sh | 48 +++++++++++++++++++++++++++++++++++++++++++++ misc/travis/clang-format.sh | 8 +++++--- 2 files changed, 53 insertions(+), 3 deletions(-) create mode 100755 misc/travis/black-format.sh (limited to 'misc') diff --git a/misc/travis/black-format.sh b/misc/travis/black-format.sh new file mode 100755 index 0000000000..75b153f6bb --- /dev/null +++ b/misc/travis/black-format.sh @@ -0,0 +1,48 @@ +#!/bin/sh + +BLACK=black +BLACK_OPTIONS="-l 120" + +if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then + # Travis only clones the PR branch and uses its HEAD commit as detached HEAD, + # so it's problematic when we want an exact commit range for format checks. + # We fetch upstream to ensure that we have the proper references to resolve. + # Ideally we would use $TRAVIS_COMMIT_RANGE but it doesn't play well with PR + # updates, as it only includes changes since the previous state of the PR. + if [ -z "$(git remote | grep upstream)" ]; then + git remote add upstream https://github.com/godotengine/godot \ + --no-tags -f -t $TRAVIS_BRANCH + fi + RANGE="upstream/$TRAVIS_BRANCH HEAD" +else + # Test only the last commit, since $TRAVIS_COMMIT_RANGE wouldn't support + # force pushes. + RANGE=HEAD +fi + +FILES=$(git diff-tree --no-commit-id --name-only -r $RANGE | grep -v thirdparty/| grep -E "(SConstruct|SCsub|\.py)$") +echo "Checking files:\n$FILES" + +# create a random filename to store our generated patch +prefix="static-check-black" +suffix="$(date +%s)" +patch="/tmp/$prefix-$suffix.patch" + +for file in $FILES; do + "$BLACK" "$BLACK_OPTIONS" --diff "$file" | \ + sed -e "1s|--- |--- a/|" -e "2s|+++ |+++ b/|" >> "$patch" +done + +# if no patch has been generated all is ok, clean up the file stub and exit +if [ ! -s "$patch" ] ; then + printf "Files in this commit comply with the black formatting rules.\n" + rm -f "$patch" + exit 0 +fi + +# a patch has been created, notify the user and exit +printf "\n*** The following differences were found between the code to commit " +printf "and the black formatting rules:\n\n" +pygmentize -l diff "$patch" +printf "\n*** Aborting, please fix your commit(s) with 'git commit --amend' or 'git rebase -i '\n" +exit 1 diff --git a/misc/travis/clang-format.sh b/misc/travis/clang-format.sh index a6585578e1..c917744ece 100755 --- a/misc/travis/clang-format.sh +++ b/misc/travis/clang-format.sh @@ -8,8 +8,10 @@ if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then # We fetch upstream to ensure that we have the proper references to resolve. # Ideally we would use $TRAVIS_COMMIT_RANGE but it doesn't play well with PR # updates, as it only includes changes since the previous state of the PR. - git remote add upstream https://github.com/godotengine/godot \ - --no-tags -f -t $TRAVIS_BRANCH + if [ -z "$(git remote | grep upstream)" ]; then + git remote add upstream https://github.com/godotengine/godot \ + --no-tags -f -t $TRAVIS_BRANCH + fi RANGE="upstream/$TRAVIS_BRANCH HEAD" else # Test only the last commit, since $TRAVIS_COMMIT_RANGE wouldn't support @@ -41,6 +43,6 @@ fi # a patch has been created, notify the user and exit printf "\n*** The following differences were found between the code to commit " printf "and the clang-format rules:\n\n" -cat "$patch" +pygmentize -l diff "$patch" printf "\n*** Aborting, please fix your commit(s) with 'git commit --amend' or 'git rebase -i '\n" exit 1 -- cgit v1.2.3