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

Add Python linter to GitHub Actions #5083

Merged
merged 14 commits into from
Sep 17, 2024
Merged
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
2 changes: 1 addition & 1 deletion .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
1. [ ] If you've added code that should be tested, add tests
2. [ ] If you've changed APIs, update (or create!) the documentation
3. [ ] Ensure the tests pass
4. [ ] Make sure that your code lints and that you've followed [our coding style](https://github.com/kobotoolbox/kpi/blob/master/CONTRIBUTING.md)
4. [ ] Run `./python-format.sh` to make sure that your code lints and that you've followed [our coding style](https://github.com/kobotoolbox/kpi/blob/master/CONTRIBUTING.md)
5. [ ] Write a title and, if necessary, a description of your work suitable for publishing in our [release notes](https://community.kobotoolbox.org/tag/release-notes)
6. [ ] Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
7. [ ] Open an issue in the [docs](https://github.com/kobotoolbox/docs/issues/new) if there are UI/UX changes
Expand Down
18 changes: 18 additions & 0 deletions .github/workflows/darker.yml
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"
7 changes: 6 additions & 1 deletion dependencies/pip/dev_requirements.in
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ pytest-cov
pytest-django
pytest-env


# Kobocat
httmock
simplejson

# Formatter and linters
ruff
flake8
flake8-quotes
darker[isort]
43 changes: 43 additions & 0 deletions dependencies/pip/dev_requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ billiard==4.2.0
# via
# -r dependencies/pip/requirements.in
# celery
black==24.8.0
# via darker
boto3==1.34.75
# via
# django-amazon-ses
Expand Down Expand Up @@ -86,6 +88,7 @@ charset-normalizer==3.3.2
# via requests
click==8.1.7
# via
# black
# celery
# click-didyoumean
# click-plugins
Expand Down Expand Up @@ -114,6 +117,12 @@ cryptography==42.0.5
# pyopenssl
cssselect==1.2.0
# via pyquery
darker[isort]==2.1.1
# via -r dependencies/pip/dev_requirements.in
darkgraylib==1.2.1
# via
# darker
# graylint
ddt==1.7.2
# via -r dependencies/pip/dev_requirements.in
decorator==5.1.1
Expand Down Expand Up @@ -252,6 +261,12 @@ executing==2.0.1
# via stack-data
fabric==3.2.2
# via -r dependencies/pip/dev_requirements.in
flake8==7.1.1
# via
# -r dependencies/pip/dev_requirements.in
# flake8-quotes
flake8-quotes==3.4.0
# via -r dependencies/pip/dev_requirements.in
flower==2.0.1
# via -r dependencies/pip/requirements.in
frozenlist==1.4.1
Expand Down Expand Up @@ -304,6 +319,8 @@ googleapis-common-protos==1.63.0
# via
# google-api-core
# grpcio-status
graylint==1.1.1
# via darker
grpcio==1.62.1
# via
# google-api-core
Expand All @@ -330,6 +347,8 @@ ipython==8.12.3
# via -r dependencies/pip/dev_requirements.in
isodate==0.6.1
# via azure-storage-blob
isort==5.13.2
# via darker
jedi==0.19.1
# via ipython
jmespath==1.0.1
Expand Down Expand Up @@ -363,6 +382,8 @@ markupsafe==2.1.5
# via werkzeug
matplotlib-inline==0.1.6
# via ipython
mccabe==0.7.0
# via flake8
mock==5.1.0
# via -r dependencies/pip/dev_requirements.in
model-bakery==1.17.0
Expand All @@ -375,6 +396,8 @@ multidict==6.0.5
# via
# aiohttp
# yarl
mypy-extensions==1.0.0
# via black
ndg-httpsclient==0.5.1
# via -r dependencies/pip/requirements.in
numpy==1.24.4
Expand All @@ -390,6 +413,7 @@ openpyxl==3.0.9
# pyxform
packaging==24.0
# via
# black
# mongomock
# pytest
pandas==2.0.3
Expand All @@ -402,12 +426,16 @@ path==16.10.0
# via path-py
path-py==12.5.0
# via formpack
pathspec==0.12.1
# via black
pexpect==4.9.0
# via ipython
pickleshare==0.7.5
# via ipython
pillow==10.3.0
# via django-markdownx
platformdirs==4.3.2
# via black
pluggy==1.4.0
# via pytest
prometheus-client==0.20.0
Expand Down Expand Up @@ -445,8 +473,12 @@ pyasn1==0.6.0
# rsa
pyasn1-modules==0.4.0
# via google-auth
pycodestyle==2.12.1
# via flake8
pycparser==2.22
# via cffi
pyflakes==3.2.0
# via flake8
pygments==2.17.2
# via
# -r dependencies/pip/requirements.in
Expand Down Expand Up @@ -540,6 +572,8 @@ rpds-py==0.18.0
# referencing
rsa==4.9
# via google-auth
ruff==0.6.2
# via -r dependencies/pip/dev_requirements.in
s3transfer==0.10.1
# via boto3
sentinels==1.0.0
Expand Down Expand Up @@ -575,8 +609,13 @@ stripe==4.2.0
# via dj-stripe
tabulate==0.9.0
# via -r dependencies/pip/requirements.in
toml==0.10.2
# via
# darker
# darkgraylib
tomli==2.0.1
# via
# black
# coverage
# pytest
# pytest-env
Expand All @@ -593,6 +632,7 @@ typing-extensions==4.10.0
# asgiref
# azure-core
# azure-storage-blob
# black
# jwcrypto
# psycopg
tzdata==2024.1
Expand Down Expand Up @@ -642,4 +682,7 @@ yarl==1.9.4
# via aiohttp
yubico-client==1.13.0
# via django-trench

# The following packages are considered to be unsafe in a requirements file:
Copy link
Contributor

Choose a reason for hiding this comment

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

Accidental change?

Copy link
Contributor Author

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

Copy link
Contributor

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.

# setuptools
backports-zoneinfo==0.2.1; python_version < '3.9'
55 changes: 55 additions & 0 deletions format-python.sh
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
39 changes: 34 additions & 5 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 #noqa or even worse #fmt off/on.

Copy link
Contributor

@rgraber rgraber Sep 10, 2024

Choose a reason for hiding this comment

The 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 #fmt off anywhere we are purposefully ignoring formatting rules

Copy link
Contributor

Choose a reason for hiding this comment

The 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 #fmt off and an explanation

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 #fmt on/off each time my line is just 2 characters above 80.

[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"
Copy link
Contributor

Choose a reason for hiding this comment

The 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',
Expand Down
Loading