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
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions .github/workflows/format.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
name: Format check

on:
pull_request:
branches:
- main
paths:
- '**.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...

runs-on: ubuntu-latest
outputs:
files: ${{ steps.git-diff-filter.outputs.files }}
name: Detect added and changed files

steps:
- uses: actions/checkout@v4
with:
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.

with:
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...


- 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"

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".

run: |
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:

needs: generate
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.

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!

name: Check file ${{ matrix.files.path }}
steps:
- uses: actions/checkout@v4
- uses: astral-sh/ruff-action@v1
with:
args: "format --check --diff"
src: "${{ matrix.files.path }}"
- if: ${{ failure() }}
run: |
echo "::warning file=${{ matrix.files.path }},title=File format check failed::Run 'ruff format ${{ matrix.files.path }}'"