build: integrate pre-commit hooks with upstream files filtering support #1121
Conversation
…tering
- Add .pre-commit-config.yaml.
- Configured hooks:
- Baseline golangci-lint (v1) at pre-commit using .golangci.yml at
pre-commit stage.
- [COMMENTED OUT] Extra golangci-lint via scripts/lint.sh at pre-commit stage.
- Full repo suite at pre-push stage.
- Standard pre-commit-hooks:
- trailing-whitespace
- end-of-file-fixer
- check-merge-conflict
- check-yaml
- check-toml
- check-json
- mdformat with mdformat-gfm and mdformat-frontmatter
- Exclude intentionally invalid fixtures: rpc/testdata/invalid-*.json.
- Add support for filtering filenames based on scripts/upstream_files.txt (non-negated entries).
- Add support for skipping directories, non-text files (file/grep heuristic), and .bin files.
- Update scripts/shellcheck.sh and scripts/actionlint.sh to avoid unbound vars under set -u.
resolves #1120
Signed-off-by: Tsvetan Dimitrov ([email protected])
powerslider
force-pushed
the
powerslider/1120-pre-commit-hooks
branch
from
cb17607 to
4a571be
Compare
powerslider
force-pushed
the
powerslider/1120-pre-commit-hooks
branch
from
a754142 to
250c431
Compare
powerslider
force-pushed
the
powerslider/1120-pre-commit-hooks
branch
from
3e0559c to
c82a81b
Compare
powerslider
force-pushed
the
powerslider/1120-pre-commit-hooks
branch
3 times, most recently
from
d362beb to
f81db78
Compare
powerslider
force-pushed
the
powerslider/1120-pre-commit-hooks
branch
from
f81db78 to
ea3388d
Compare
| exclude: ^rpc/testdata/invalid-.*\.json$ | ||
|
|
||
| # Markdown formatter | ||
| - repo: https://github.com/hukkin/mdformat |
There was a problem hiding this comment.
What's standard here? I'm not sure if we want to make these changes just for the pre-commit hooks (added arbitrarily) if we're not also going to lint for them
There was a problem hiding this comment.
This is the style guide it follows https://mdformat.readthedocs.io/en/stable/users/style.html
There was a problem hiding this comment.
I agree with @alarso16 re aligning pre-commit and linter checks. IMO the pre-commit is the fast-fail while the linter is the gatekeeper.
powerslider
force-pushed
the
powerslider/1120-pre-commit-hooks
branch
from
56d2b9e to
bcda734
Compare
powerslider
force-pushed
the
powerslider/1120-pre-commit-hooks
branch
3 times, most recently
from
aeaa6d9 to
b13b98b
Compare
powerslider
force-pushed
the
powerslider/1120-pre-commit-hooks
branch
from
b13b98b to
26c4001
Compare
…ring for the hooks
powerslider
force-pushed
the
powerslider/1120-pre-commit-hooks
branch
from
26c4001 to
4819f5c
Compare
There was a problem hiding this comment.
(fyi) I'm not reviewing this file, on the assumption that it's only formatting. Please let me know if that's an incorrect assumption.
| // | ||
| // Much love to the original authors for their work. | ||
| // ********** | ||
|
|
There was a problem hiding this comment.
I'm 99% sure that this is not what we want. By removing the blank space it will force the copyright header to become part of the package comment. Run godoc locally to check.
I suspect that the header hasn't been configured correctly.
There was a problem hiding this comment.
As above, not reviewing as assuming that it's only formatting.
| 4. An off-chain relayer queries the validators for their signatures of the message and aggregates the signatures to create a `SignedMessage` | ||
| 5. The off-chain relayer encodes the `SignedMessage` as the [predicate](#predicate-encoding) in the AccessList of a transaction to deliver on blockchain B | ||
| 6. The transaction is delivered on blockchain B, the signature is verified prior to executing the block, and the message is accessible via the Warp Precompile's `getVerifiedWarpMessage` during the execution of that transaction | ||
| 1. Warp Precompile emits an event / log containing the `UnsignedMessage` specified by the caller of `sendWarpMessage` |
There was a problem hiding this comment.
Is there no linter that can instead check that the numbers are up to date? This forces someone to refer to the rendered version, which defeats the point of markdown being readable.
There was a problem hiding this comment.
Why are there 2 new markdown files in this directory?
| set -euo pipefail | ||
|
|
||
| go run github.com/rhysd/actionlint/cmd/[email protected] "${@}" | ||
| if [[ $# -gt 0 ]]; then |
There was a problem hiding this comment.
Is this a shellcheck recommendation? My bash-fu isn't strong enough to review this comfortably.
| done | ||
|
|
||
| find "${REPO_ROOT}" \( "${IGNORED_CONDITIONS[@]}" \) -o -type f -name "*.sh" -print0 | xargs -0 "${SHELLCHECK}" "${@}" | ||
| if [[ $# -gt 0 ]]; then |
There was a problem hiding this comment.
As with run_task.sh, why this pattern? Did shellcheck recommend it?
| exclude: ^rpc/testdata/invalid-.*\.json$ | ||
|
|
||
| # Markdown formatter | ||
| - repo: https://github.com/hukkin/mdformat |
There was a problem hiding this comment.
I agree with @alarso16 re aligning pre-commit and linter checks. IMO the pre-commit is the fast-fail while the linter is the gatekeeper.
There was a problem hiding this comment.
I don't think we should be introducing even more bash scripts when a Go binary can do the same job, but in a much more readable fashion. Why not have the pre-commit entrypoint be something like go run ./scripts/foo --checker="bar"?
If we want to avoid a new directory for every script then we could just create a cobra app and add scripts/main.go as the entry and a new Go file with separate sub-command for what otherwise would have been a new shell script. Then the entrypoint would be go run ./scripts precommit filter --checker="bar".
4 participants
Why this should be merged
Check #1120 for the motivations.
How this works
golangci-lint(v1) at pre-commit using.golangci.ymlat pre-commit stage.trailing-whitespaceend-of-file-fixercheck-merge-conflictcheck-yamlcheck-tomlcheck-jsonmdformatwithmdformat-gfmandmdformat-frontmatterrpc/testdata/invalid-*.json.scripts/shellcheck.shandscripts/actionlint.shto avoid unbound vars underset -u.How this was tested
Manually by installing the
pre-commithooks and running them at pre-commit stage.Need to be documented?
Only dev documentation.
Need to update RELEASES.md?
no
resolves #1120
Signed-off-by: Tsvetan Dimitrov ([email protected])