ci: Add format check workflow #766
Conversation
.github/workflows/format.yml
Outdated
| continue-on-error: true | ||
| with: | ||
| args: "format --check" | ||
| changed-files: "true" |
There was a problem hiding this comment.
Nice.
I checked the source code and this relies on https://github.com/tj-actions/changed-files
Years ago, I could not find anything like it and simple and reliable so hopefully this one does the job.
I wrote this git script at the time: thesofproject/sof-test@336094b
Add setting for the ruff formatter to check during CI. Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
e75081d
to
32b9e1b
Compare
|
Went a step further, new files are always fully checked, changed chunks now also need to be formatted. Experimenting shown here: https://github.com/pdgendt/west/actions/workflows/format.yml with annotations |
edd1648
to
c9c21a7
Compare
cc4530a
to
a487d53
Compare
|
Checking only modified chunks sounds great in theory but I'm concerned that the code of the Github workflow is getting too complicated and hard to maintain. It's not like As long as this check does not block merges, I think checking only new and modified files (the entire file each time) is a good enough trade-off. For sure that trade-off has been working well for years with |
a487d53
to
a27015a
Compare
Updated the PR accordingly:
|
Add a CI workflow to report formatting issues on changed files. This is to gradually update files in the repository to be conform with PEP8 formatting. Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
a27015a
to
5ea42a8
Compare
Formatting test_alias.py is trivial. Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
Formatting test_commands.py is trivial. Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
4a9c26e
to
c9af293
Compare
| - '**.py' | ||
|
|
||
| jobs: | ||
| generate: |
There was a problem hiding this comment.
I think this name is part of the "user interface" so can it be a bit longer and more specific? find-changed-files or better...
| fetch-depth: 0 | ||
|
|
||
| - uses: GrantBirki/git-diff-action@v2 | ||
| id: git-diff-action |
There was a problem hiding this comment.
Can you make this name (slightly) different from the action name? Right now it's confusing to tell which object is being referred to.
| # Ignore deleted files | ||
| git_options: '--no-color --diff-filter=d' | ||
|
|
||
| - name: Filter json diff |
There was a problem hiding this comment.
Filter what? It should be possible to have a clue without reading the code.
There was a problem hiding this comment.
Doesn't file_output_only do this? It's "highly recommended"
| files=$(echo $JSON_DIFF | jq -c -r '[.files[] | {path: .path}]') | ||
| echo "files=$files" >> $GITHUB_OUTPUT | ||
|
|
||
| check-files: |
There was a problem hiding this comment.
| check-files: | |
| ruff-format: |
| continue-on-error: true | ||
| strategy: | ||
| matrix: | ||
| files: ${{ fromJSON(needs.generate.outputs.files) }} |
| if: ${{ needs.generate.outputs.files != '[]' }} | ||
| runs-on: ubuntu-latest | ||
| # Allow the workflow run to pass when this job fails | ||
| continue-on-error: true |
There was a problem hiding this comment.
Does this just change red to green? If yes then I don't like it, sorry for the confusion.
https://github.com/orgs/community/discussions/15452
If formatting fails then the check should be red to draw attention, then maintainers should be able to merge anyway.
| - name: Filter json diff | ||
| id: git-diff-filter | ||
| env: | ||
| JSON_DIFF: ${{ steps.git-diff-action.outputs.json-diff }} |
There was a problem hiding this comment.
Using json_diff_file_output is "highly recommended".
| base_branch: origin/main | ||
| search_path: '**.py' | ||
| # Ignore deleted files | ||
| git_options: '--no-color --diff-filter=d' |
There was a problem hiding this comment.
Are you really sure --no-color is required? It's very surprising not get this by default in such a context...
Add a CI workflow to report formatting issues on changed files. This is to gradually update files in the repository to be conform with PEP8 formatting.