-
-
Notifications
You must be signed in to change notification settings - Fork 186
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
Add Python linter to GitHub Actions #5083
Changes from all commits
7e7d77f
7786f73
14e7b85
6f281ca
f4dccde
ffe8b02
127118d
4245137
8b53e78
32421e8
499a7c0
5e07fe9
5ce6eab
f9846a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
name: Python linter (Darker) | ||
|
||
on: [pull_request] | ||
jobs: | ||
darker: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v4 | ||
with: | ||
fetch-depth: 0 | ||
- uses: actions/setup-python@v5 | ||
- name: Install pip dependencies | ||
run: python -m pip install flake8 flake8-quotes isort | ||
- uses: akaihola/darker@master | ||
with: | ||
options: '--check --isort -L "flake8 --max-line-length=88"' | ||
revision: "origin/${{ github.event.pull_request.base.ref }}" | ||
version: "~=2.1.1" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
#!/bin/bash | ||
|
||
BASE_REVISION=$1 | ||
|
||
if [ -n "${UWSGI_USER}" ] && [ "${DEBIAN_FRONTEND}" == "noninteractive" ] && [ "${TMP_DIR}" == "/srv/tmp" ]; then | ||
INSIDE_CONTAINER=1 | ||
else | ||
INSIDE_CONTAINER=0 | ||
fi | ||
|
||
if [ -z "$BASE_REVISION" ]; then | ||
BASE_REVISION="origin/beta" | ||
elif [ "$BASE_REVISION" == "-l" ] || [ "$BASE_REVISION" == "--last" ]; then | ||
if [ "$INSIDE_CONTAINER" == "1" ]; then | ||
BASE_REVISION=$(gosu "$UWSGI_USER" git log --oneline| head -n 1 | awk '{ print $1}') | ||
else | ||
BASE_REVISION=$(git log --oneline| head -n 1 | awk '{ print $1}') | ||
fi | ||
fi | ||
|
||
# First, do not touch formatting but fix: | ||
# - single/double quotes (--select Q) | ||
# - sort imports (--select I) | ||
# - unused imports (--select F401) | ||
# - deprecated `mock` (--select UP026) | ||
# - extraneous-parentheses (--select UP034) | ||
# - Unnecessary parentheses after class definition (--select UP039) | ||
# - Indentation warning (--select W1) | ||
# - No newline at end of file (--select W292) | ||
if [ "$INSIDE_CONTAINER" == "1" ]; then | ||
PYTHON_CHANGES=$(gosu "$UWSGI_USER" git diff --name-only "$BASE_REVISION" | grep '\.py') | ||
else | ||
PYTHON_CHANGES=$(git diff --name-only "$BASE_REVISION" | grep '\.py') | ||
fi | ||
|
||
if [ -n "$PYTHON_CHANGES" ]; then | ||
echo "Using ruff..." | ||
if [ "$INSIDE_CONTAINER" == "1" ]; then | ||
gosu "$UWSGI_USER" git diff --name-only "$BASE_REVISION" | grep '\.py' | xargs ruff check --select Q --select I --select F401 --select UP026 --select UP034 --select UP039 --select W292 --fix | ||
else | ||
git diff --name-only "$BASE_REVISION" | grep '\.py' | xargs ruff check --select Q --select I --select F401 --select UP026 --select UP034 --select UP039 --select W292 --fix | ||
fi | ||
|
||
# Applying Black format with `darker` with options: | ||
# --isort: Using isort | ||
# --revision: Compare changes with revision $BASE_REVISION | ||
echo "Using darker..." | ||
if [ "$INSIDE_CONTAINER" == "1" ]; then | ||
gosu "$UWSGI_USER" darker --isort --revision "$BASE_REVISION" | ||
else | ||
darker --isort --revision "$BASE_REVISION" | ||
fi | ||
else | ||
echo "No Python changes detected!" | ||
fi |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,24 +4,53 @@ | |
# Do not enable `verbose = true` unless Black changes their behavior to respect | ||
# `--quiet` on the command line properly | ||
# verbose = true | ||
line-length = 80 | ||
line-length = 88 | ||
skip-string-normalization = true | ||
|
||
[tool.darker] | ||
src = [ | ||
"kobo", | ||
"hub", | ||
"kpi", | ||
] | ||
isort = true | ||
|
||
[tool.isort] | ||
profile = "black" | ||
line_length = 80 # same as black | ||
line_length = 88 # same as black | ||
known_first_party = "kobo" | ||
no_lines_before = ["LOCALFOLDER"] | ||
|
||
[tool.ruff] | ||
line-length = 80 | ||
line-length = 88 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're capping this at 88 we should cap black and isort at 88 as well, or put a comment explaining why they are different. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know we discussed this on an earlier PR, but I still don't understand why we kept our black config at 80 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want the formatter to break at 80 but we don't want the linter to complain if we manually keep in purpose at line between 80 and 90 (as explained before) without adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should discuss this more at a later date. I think we should decide on a hard line limit and add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To put it another way: I think we need to decide firmly whether 88 is an acceptable limit. If it is, it should be the limit in all cases and we don't need to warn at 80. If not, we should standardize the line-length at 80 and explicitly call out lines that break that rule with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's not waste our time on this. Let's use 88 everywhere since that's the default for Black and ruff. Moreover, I really don't want to add |
||
[tool.ruff.format] | ||
quote-style = "single" # Preserve is coming soon to ruff | ||
[tool.ruff.lint] | ||
# Enable ruff isort | ||
extend-select = ["I"] | ||
extend-select = [ | ||
"I", # Enable ruff isort | ||
"Q", # Flake quotes | ||
"E", # pycodestyle, some needs `--preview` to be enable | ||
"N", # PEP-8 naming convention | ||
"UP026", # deprecated mock | ||
"UP034", # extraneous-parentheses | ||
"UP039", # Unnecessary parentheses after class definition | ||
"W1", # Indentation warning | ||
"W292", # no newline at end of file | ||
"T20", # (p)print found | ||
] | ||
[tool.ruff.lint.flake8-quotes] | ||
inline-quotes = "single" # To prefer single quotes over double quote | ||
multiline-quotes = "double" | ||
docstring-quotes = "double" | ||
[tool.ruff.lint.isort] | ||
known-first-party = ["kobo"] | ||
|
||
[tool.flake8] | ||
inline-quotes = "single" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. irony |
||
multiline-quotes = "double" | ||
docstring-quotes = "double" | ||
max-line-length = 88 | ||
|
||
[tool.pytest.ini_options] | ||
testpaths = [ | ||
'kobo', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidental change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, produced by
./pip-compile.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird. It seems unrelated to the changes, which is a little worrisome.