-
Notifications
You must be signed in to change notification settings - Fork 13
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
Adds linting via pre-commit #62
Open
wusatosi
wants to merge
14
commits into
bemanproject:main
Choose a base branch
from
wusatosi:lint
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+542
−276
Open
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
afad160
propogate lint infrastructure
wusatosi c22bb8f
customize pre-commit to fit beman
wusatosi 5ccdca1
Format to conform with pre-commit config
wusatosi e8da514
clang-format is not needed
wusatosi 385634b
use pre-commit as single source of truth for codespell
wusatosi bf3bc01
fix document links (lower case -> upper case) (#63)
dietmarkuehl d3fe785
Docs updates: org renamed to bemanproject
neatudarius 50b701d
add git blame ignore file
wusatosi 5a0f3c8
format trailing whitespace
wusatosi 316a459
bump dependencies
wusatosi f08f7c9
revert incorrect merge
wusatosi 9815e13
Merge branch 'main' into lint
wusatosi 57b9446
Format HEAD
wusatosi 59561bd
include format into blame ignore
wusatosi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| 64168e2e0e9d3ef2fe93fcb6adb8e5d2b020eda1 | ||
| 57b9446ff233660e8668e76b9c27f995ba5eb5f8 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| name: Lint Check (pre-commit) | ||
|
|
||
| on: | ||
| # We have to use pull_request_target here as pull_request does not grant | ||
| # enough permission for reviewdog | ||
| pull_request_target: | ||
| push: | ||
|
|
||
| jobs: | ||
| pre-commit-push: | ||
| name: Pre-Commit check on Push | ||
| runs-on: ubuntu-latest | ||
| if: ${{ github.event_name == 'push' }} | ||
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: 3.13 | ||
|
|
||
| # We wish to run pre-commit on all files instead of the changes | ||
| # only made in the push commit. | ||
| # | ||
| # So linting error persists when there's formatting problem. | ||
| - uses: pre-commit/[email protected] | ||
|
|
||
| pre-commit-pr: | ||
| name: Pre-Commit check on PR | ||
| runs-on: ubuntu-latest | ||
| if: ${{ github.event_name == 'pull_request_target' }} | ||
|
|
||
| permissions: | ||
| contents: read | ||
| checks: write | ||
| issues: write | ||
| pull-requests: write | ||
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
||
| # pull_request_target checkout the base of the repo | ||
| # We need to checkout the actual pr to lint the changes. | ||
| - name: Checkout pr | ||
| run: gh pr checkout ${{ github.event.number }} | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: 3.13 | ||
|
|
||
| # we only lint on the changed file in PR. | ||
| - name: Get Changed Files | ||
| id: changed-files | ||
| uses: tj-actions/changed-files@v45 | ||
|
|
||
| # See: | ||
| # https://github.com/tj-actions/changed-files?tab=readme-ov-file#using-local-git-directory- | ||
| - uses: pre-commit/[email protected] | ||
| id: run-pre-commit | ||
| with: | ||
| extra_args: --files ${{ steps.changed-files.outputs.all_changed_files }} | ||
|
|
||
| # Review dog posts the suggested change from pre-commit to the pr. | ||
| - name: suggester / pre-commit | ||
| uses: reviewdog/action-suggester@v1 | ||
| if: ${{ failure() && steps.run-pre-commit.conclusion == 'failure' }} | ||
| with: | ||
| tool_name: pre-commit | ||
| level: warning | ||
| reviewdog_flags: "-fail-level=error" | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| # MD033/no-inline-html : Inline HTML : https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md033.md | ||
| # Disable inline html linter is needed for <details> <summary> | ||
| MD033: false | ||
|
|
||
| # MD013/line-length : Line length : https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md013.md | ||
| # General Column Limit for code is 119. | ||
| MD013: | ||
| code_block_line_length: 119 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| # See https://pre-commit.com for more information | ||
| # See https://pre-commit.com/hooks.html for more hooks | ||
| repos: | ||
| - repo: https://github.com/pre-commit/pre-commit-hooks | ||
| rev: v5.0.0 | ||
| hooks: | ||
| - id: trailing-whitespace | ||
| - id: end-of-file-fixer | ||
| - id: check-yaml | ||
| - id: check-added-large-files | ||
|
|
||
| # CMake linting and formatting | ||
| - repo: https://github.com/BlankSpruce/gersemi | ||
| rev: 0.17.1 | ||
| hooks: | ||
| - id: gersemi | ||
| name: CMake linting | ||
|
|
||
| # Markdown linting | ||
| # Config file: .markdownlint.yaml | ||
| - repo: https://github.com/igorshubovych/markdownlint-cli | ||
| rev: v0.43.0 | ||
| hooks: | ||
| - id: markdownlint | ||
|
|
||
| # Code spelling check, note auto-correction is not enabled | ||
| - repo: https://github.com/codespell-project/codespell | ||
| rev: v2.2.4 | ||
| hooks: | ||
| - id: codespell | ||
| exclude: ^build|presentations |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm wonder if this is really what we want? Aside from the obvious inefficiencies involved in checking all files all the time this will make it potentially difficult to override a lint/format setting. Let me give you a concrete case of this. In my day-job-project we using clang-tidy on code, but it struggles in any setup we've used to nicely format monadic function chains. So if we had are setup run all files every checkin it would be annoying (note there's likely ways to turn off the changes we don't know about). Looking for other opinions on this point.
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.
Upstream for this CI config is in
exemplar, so the context is this is meant to be run along side some huge unit test complex.The main idea is if we have a linting rule (or any enforceable rule) then if some part of the system is broken, the whole system is broken. Linting potentially causes sweeping changes due to reformatting, so its best the reformat is as close to lint error as possible (best in the same commit).
Explicitly, we are trying to avoid this:
Here the lint failure in 0 may still not be fixed because no diff later touches a.cpp .
codespell:ignore,markdownlint-disable-next-line,gersemi: ignore. Linting error should be explicitly suppressed (best in-line), but not to be ignored. Running lint check only on diff doesn't guarantee error propagation unless we do complicated trickery. Think this as unit test, you would ideally run the entire unit-test set, and the solution to flaky unit tests are, ideally to fix them first, if it doesn't work then skip, but not to ignore them from CI perspective out-right.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.
Thanks for the long explanation. And while I agree there are overrides possible many of these involve actual source modifications which are sometimes unpleasant for authors, etc. And, if I'm not mistaken the standard we're setting here will require the revisiting of those standards on each commit for unchanged source.
Sure. I'll note however, that none of this is explained in exemplar at the moment.
To me there's a large difference between a broken test and some code that doesn't pass formatting checks. So maybe that makes these 'less enforceable rules'.
I guess at a minimum I'd like some others to chime in on this.
@steve-downey @neatudarius @camio - thoughts?
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.
I agree with @wusatosi that enforcement of linter checks at CI time is the most desirable option and this is what we're doing with every other repository in beman so far. We "push left" by allowing developers to get this feedback with the pre-commit tool, but if they don't use this, there's review dog to suggest the changes needed at PR time. I've worked in several repositories that enforce linters and it's been a positive experience.