Skip to content

Commit

Permalink
Adding linter and formatter pre-commit hooks for staged BE files
Browse files Browse the repository at this point in the history
* Adding linter and formatter to staged BE files.

Note that `.cljfmt/indents.clj` tries to capture our indent rules
and, so far, seems to get things right. There are still some inconsistencies
in some of our nses in how we indent assoc(-in) and some t2 functions
when it comes to key-value pairs on following lines. Should the keys
align with the function call or be indented. Aesthetically, people
seem to like indents, but these are functions, so should align with the
first argument. IDK that it really matters as long as we have agreement.

We may, as we adopt this, have some files be reformatted in unexpected ways
for items that have been missed. If so, the developer can simply update
the .cljfmt/indents.clj file, rerun the hook, and commit that along with
their other changes.

* Added whitespace linter.

* Updating husky commit hook

* Updated hook scripts to be exclusively for staged files.

Updated cljfmt to use the latest and greatest along with the new format of config file.

* Commenting out the actual formatting hook in .husky/pre-commit until we come up with a globally happy solution to forms that don't have good formatting rules, like defprotocol+ in a reader conditional.

* Reverting formatting

* Moving pre-commit hooks from .husky/pre-commit to the package.json's lint-staged section.

Note that we're still not calling the cljfmt file (.bin/cljfmt_staged.sh)
until we get agreement on formatting.

One thing we might do since we're using lint-staged is we can probably
exclude the "bad" files that go sideways with linting and format everything
else automatically. This might be a good follow-on PR.
  • Loading branch information
markbastian authored Nov 14, 2023
1 parent f0f4e7f commit d4957af
Show file tree
Hide file tree
Showing 8 changed files with 100 additions and 11 deletions.
31 changes: 31 additions & 0 deletions .cljfmt.edn
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
;; https://github.com/weavejester/cljfmt/blob/master/cljfmt/resources/cljfmt/indents/clojure.clj
{:extra-indents
{auto-parse [[:inner 0]]
cond-> [[:inner 0]]
check [[:inner 0]]
deferred-tru [[:inner 0]]
defprotocol+ [[:block 1] [:inner 1]]
potemkin/defprotocol+ [[:block 1] [:inner 1]]
let-404 [[:block 1]]
metabase.mbql.util/match-one [[:inner 0]]
merge-with [[:inner 0]]
metabase.test/dataset [[:inner 0]]
metabase.test/formatted-rows [[:inner 0]]
metabase.test/mbql-query [[:inner 0]]
metabase.test/test-drivers [[:inner 0]]
metabase.test/with-temporary-setting-values [[:block 1]]
;; If we just treat these as functions we can ignore this, despite some files not being consistent
;metabase.models.permissions/set-has-full-permissions-for-set? [[:inner 0]]
;metabase.models.permissions/set-has-full-permissions? [[:inner 0]]
;metabase.models.permissions/set-has-partial-permissions? [[:inner 0]]
metabase.models.permissions/set-has-partial-permissions-for-set? [[:inner 0]]
publish-event! [[:block 1]]
metabase.query-processor.streaming/streaming-response [[:inner 0]]
metabase.util/prog1 [[:inner 0]]
metabase.util/select-keys-when [[:inner 0]]
;; This supports a lot of existing formatting, but these are functions so the
;; existing formatting is probably wrong.
;toucan2.core/select [[:inner 0]]
;toucan2.core/update! [[:inner 0]]
}
:sort-ns-references? true}
2 changes: 0 additions & 2 deletions .cljfmt/indents.clj

This file was deleted.

9 changes: 8 additions & 1 deletion .husky/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,11 @@
. "$(dirname "$0")/_/husky.sh"

yarn precommit
./hooks/pre-commit.nocommit

# Run the formatter
# Note that this is stubbed in for now until we can gobally resolve some formatting issues such as what is described
# here: https://github.com/metabase/metabase/pull/35511#pullrequestreview-1721758128
# ATM, you can manually run ./bin/cljfmt_staged.sh if you want to format locally staged files and inspect after.
# ./bin/cljfmt_staged.sh

./hooks/pre-commit.nocommit
7 changes: 0 additions & 7 deletions bin/cljfmt.sh

This file was deleted.

31 changes: 31 additions & 0 deletions bin/cljfmt_staged.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#! /usr/bin/env bash

# Reformat staged clj, cljc, and cljs files
# Will print the results of the call. Will return an error (stopping the commit stage) if any files are reformatted.
#
# This is not a custom reformatter.
# In the event that do you want to do some manual reformatting, you can do this:
# clojure -T:cljfmt fix ;; Reformat everything

set -euo pipefail

STAGED_FILES=$(git diff --name-status --cached -- "*.clj" "*.cljc" "*.cljs" | grep -E '[AM]' | cut -f2 || true)

if [ "${#STAGED_FILES}" -gt 0 ]; then
args=()
for file in $STAGED_FILES; do
args+=("\"$file\"")
done

output=$(clojure -T:cljfmt fix "{:paths [${args[*]}]}" 2>&1)

## Return a non-zero error code if any files were formatted since this will cause the commit to abort
if [ -n "$output" ]; then
echo $output
exit 1
else
echo "All staged clj, cljc, or cljs formatted correctly"
fi
else
echo "No staged clj, cljc, or cljs files to format"
fi
20 changes: 20 additions & 0 deletions bin/whitespace_lint_staged.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#! /usr/bin/env bash

# Do whitespace linting on passed in files.
# This is intended to be called from a commit hook (see package.json > lint-staged) that passes in staged changes

set -euo pipefail

STAGED_FILES="$@"

if [ "${#STAGED_FILES}" -gt 0 ]; then
args=()
for file in $STAGED_FILES; do
args+=("\"$file\"")
done
echo checking formatting for $STAGED_FILES

clojure -T:whitespace-linter lint "{:paths [${args[*]}]}"
else
echo "No staged clj, cljc, cljs, or edn files to whitespace lint."
fi
7 changes: 6 additions & 1 deletion deps.edn
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,12 @@
;; default heap size is 1GB, we were getting OOM on CI so we need to bump this
"-Xmx4g"]}

;;; building Uberjar, build and release scripts
;; clj -T:cljfmt fix '{:paths ["src/metabase/events/view_log.clj"]}'
:cljfmt
{:deps {io.github.weavejester/cljfmt {:git/tag "0.11.2", :git/sha "fb26b22f569724b05c93eb2502592dfc2de898c3"}}
:ns-default cljfmt.tool}

;;; building Uberjar, build and release scripts

:build
{:extra-paths
Expand Down
4 changes: 4 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,10 @@
"e2e/test/scenarios/*/{*.js,!(helpers|shared)/*.js}": [
"eslint --rulesdir frontend/lint/eslint-rules --max-warnings 0 --fix",
"node e2e/validate-e2e-test-files.js"
],
"**/*.{clj,cljc,cljs,bb}": [
"clj-kondo --config ./.clj-kondo/config.edn --config-dir ./.clj-kondo --parallel --lint",
"./bin/whitespace_lint_staged.sh"
]
},
"browserslist": [
Expand Down

0 comments on commit d4957af

Please sign in to comment.