-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
Conversation
695038d
to
6f281ca
Compare
32aaced
to
8b53e78
Compare
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.
Can we update PULL_REQUEST_MD as well? Something like
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).
@@ -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: |
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.
[tool.isort] | ||
profile = "black" | ||
line_length = 80 # 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 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.
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.
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 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
.
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.
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
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.
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
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.
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.lint.isort] | ||
known-first-party = ["kobo"] | ||
|
||
[tool.flake8] | ||
inline-quotes = "single" |
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.
irony
format-python.sh
Outdated
|
||
# Applying Black format on changes (since $BASE_REVISION) | ||
echo "Using darker..." | ||
darker -i -r "$BASE_REVISION" |
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.
A comment here about what -i and -r do would be helpful
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.
I replaced with long name options to make it more obvious.
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.
LGTM
Description
Use
darker
withflake8
to lint changes (only) in Python code.Notes
An utility has been created in this PR to be used before submitting new commits. It has not been added to pre-commit hook to give the developer the liberty to edit changes make by the formatter before pushing their changes.
It uses
ruff
first, thendarker
.ruff
should only check (unused and/or unsorted) imports, single vs double quotes and some misc other rules. This should apply on the whole content of every edited or new file of a PR.The main formatting is delegated to
darker
. This should apply only on parts that changed of every edited or new file of a PR.To run:
bash format-python.sh [revision]
By default, it looks for changes with the
beta
branch ifrevision
is not specified.-l
(L in lowercase) or--last
option can be used to make the diff with last commit in the history log.Examples:
bash format-python.sh
# use diff withorigin/beta
bash format-python.sh main
# use diff withmain
bash format-python.sh abcd1234
# use diff with commitabcd1234
bash format-python.sh -l
# use diff with last commit