Skip to content
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

ci: Add format check workflow #766

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pdgendt
Copy link
Collaborator

@pdgendt pdgendt commented Nov 4, 2024

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.

continue-on-error: true
with:
args: "format --check"
changed-files: "true"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this check be easily overriden ignored when needed? For instance we don't want one-line fix PRs to be drowned in whitespace changes.

Add setting for the ruff formatter to check during CI.

Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>
@pdgendt
Copy link
Collaborator Author

pdgendt commented Nov 6, 2024

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

@pdgendt pdgendt force-pushed the ci-ruff-format branch 2 times, most recently from edd1648 to c9c21a7 Compare November 6, 2024 09:55
@pdgendt pdgendt marked this pull request as ready for review November 6, 2024 10:02
@pdgendt pdgendt force-pushed the ci-ruff-format branch 3 times, most recently from cc4530a to a487d53 Compare November 6, 2024 13:30
@marc-hb
Copy link
Collaborator

marc-hb commented Nov 6, 2024

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 west has had an oversupply of maintainers :-/

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 pylint and shellcheck in thesofproject/sof-test@336094b

@pdgendt
Copy link
Collaborator Author

pdgendt commented Nov 8, 2024

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 west has had an oversupply of maintainers :-/

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 pylint and shellcheck in thesofproject/sof-test@336094b

Updated the PR accordingly:

  • Ignore deleted python files
  • Run ruff format --check --diff on entire files
  • Add continue-on-error: true to make the workflow pass if the format job fails (making it non-blocking)
  • Add a warning annotation instead of an error

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>
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>
- '**.py'

jobs:
generate:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filter what? It should be possible to have a clue without reading the code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
check-files:
ruff-format:

continue-on-error: true
strategy:
matrix:
files: ${{ fromJSON(needs.generate.outputs.files) }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow!

if: ${{ needs.generate.outputs.files != '[]' }}
runs-on: ubuntu-latest
# Allow the workflow run to pass when this job fails
continue-on-error: true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you really sure --no-color is required? It's very surprising not get this by default in such a context...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants