Skip to content

tests: add linter and check black #1

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

tests: add linter and check black #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions .github/workflows/tests-python.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# -*- coding: utf-8 -*-
#
# Copyright (C) 2024 CERN.
# Copyright (C) 2025 Graz University of Technology.

name: Python CI

Expand Down Expand Up @@ -55,6 +56,15 @@ jobs:
uses: ./.github/actions/pre-install
if: ${{ hashFiles('.github/actions/pre-install/action.yml') != '' }}

- uses: psf/black@stable
with:
options: "--check --verbose"

- name: Linter
uses: astral-sh/ruff-action@v3
with:
args: "check --select I,T20,F401,F841,F811"
Comment on lines +59 to +66
Copy link
Contributor

@ntarocco ntarocco Nov 3, 2025

Choose a reason for hiding this comment

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

The issue with this is that you cannot run this locally, before pushing your changes.
That's why we have the run-tests.sh and the rules configured in the setup.cfg.

What are actually checking now with ruff?
Wasn't it black + isort enough?

Copy link
Contributor Author

@utnapischtim utnapischtim Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yes we have the run-tests.sh but if we change something there, we have to apply it again on every package.

introducing this change, would improve the test workflow on github without interfering locally. in my thinking the local development checks about black in the IDE by applying black on the save file hook. further isort is applied there too. so if you run run-tests.sh locally with black and isort included it should not be a problem if it runs at the end of the pytest tests.

the same configuration, as we have locally would still work on github, but would not fail anymore since it would fail before, so we are faster.

i enabled following ruff rules:

T20 .. p(p)rint statements
F401 .. unused imports
F841 .. unused variable
F811 .. redefined while unused

which are not covered by black and isort. the I stands for isort. i included that, because ruff has it included, but we can remove that and stay on isort for it. for me i would like to include those linter rules, because some of them can hide bugs unused variable and redefined while unused.

Copy link
Member

@egabancho egabancho Nov 3, 2025

Choose a reason for hiding this comment

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

I am not against this, but we need to be aware that it will initially fail for some repositories. I randomly ran it on Invenio-Communities

ruff check --select I,T20,F401,F841,F811
...
Found 18 errors.
No fixes available (18 hidden fixes can be enabled with the `--unsafe-fixes` option).

Copy link
Contributor Author

@utnapischtim utnapischtim Nov 3, 2025

Choose a reason for hiding this comment

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

yes it will fail. if it would not fail, i would have not the reason to activate it!

i included only the bar minimum what i think we should enable. we can discuss afterwards if we want to introduce further rules. on my packages i do it the other way around, i enable all rules and ignore rules where i think they are to much or are not applicable to the invenio code base


- name: Install dependencies
run: |
pip install ".[$EXTRAS]"
Expand Down