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
Show file tree
Hide file tree
Changes from all commits
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 }}'"
4 changes: 4 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,7 @@ extend-select = [
ignore = [
"UP027", # deprecated pyupgrade rule
]

[tool.ruff.format]
quote-style = "preserve"
line-ending = "lf"
9 changes: 9 additions & 0 deletions tests/test_alias.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@

assert 'TOXTEMPDIR' in os.environ, "you must run these tests using tox"


@pytest.fixture(autouse=True)
def autouse_tmpdir(config_tmpdir, west_init_tmpdir):
# Since this module tests west's configuration file features,
# adding autouse=True to the config_tmpdir and west_init_tmpdir fixtures
# saves typing and is less error-prone than using it below in every test case.
pass


def test_alias_commands():
cmd('config alias.test1 topdir')
cmd('config --global alias.test2 topdir')
Expand All @@ -28,6 +30,7 @@ def test_alias_commands():
assert cmd('test2') == topdir_out
assert cmd('test3') == topdir_out


def test_alias_help():
cmd('config alias.test topdir')

Expand All @@ -36,13 +39,15 @@ def test_alias_help():
assert "An alias that expands to: topdir" in help_out
assert cmd('-h test') == help_out


def test_alias_recursive_commands():
list_format = '{revision} TESTALIAS {name}'
cmd(['config', 'alias.test1', f'list -f "{list_format}"'])
cmd('config alias.test2 test1')

assert cmd('test2') == cmd(['list', '-f', list_format])


def test_alias_infinite_recursion():
cmd('config alias.test1 test2')
cmd('config alias.test2 test3')
Expand All @@ -53,6 +58,7 @@ def test_alias_infinite_recursion():

assert 'unknown command "test1";' in str(excinfo.value.stdout)


def test_alias_empty():
cmd(['config', 'alias.empty', ''])

Expand All @@ -64,18 +70,21 @@ def test_alias_empty():

assert 'empty alias "empty"' in str(excinfo.value.stdout)


def test_alias_early_args():
cmd('config alias.test1 topdir')

# An alias with an early command argument shouldn't fail
assert "Replacing alias test1 with ['topdir']" in cmd('-v test1')


def test_alias_command_with_arguments():
list_format = '{revision} TESTALIAS {name}'
cmd(['config', 'alias.revs', f'list -f "{list_format}"'])

assert cmd('revs') == cmd(['list', '-f', list_format])


def test_alias_override():
before = cmd('list')
list_format = '{name} : {revision}'
Expand Down
1 change: 1 addition & 0 deletions tests/test_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

gv = WestCommand._parse_git_version


def test_parse_git_version():
# White box test for git parsing behavior.
assert gv(b'git version 2.25.1\n') == (2, 25, 1)
Expand Down