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

Conversation

noliveleger
Copy link
Contributor

@noliveleger noliveleger commented Aug 28, 2024

Description

Use darker with flake8 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, then darker. 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 if revision 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 with origin/beta
  • bash format-python.sh main # use diff with main
  • bash format-python.sh abcd1234 # use diff with commit abcd1234
  • bash format-python.sh -l # use diff with last commit

@noliveleger noliveleger marked this pull request as draft August 28, 2024 19:19
@noliveleger noliveleger added Back end enhancement Ideas, improvements and features labels Sep 9, 2024
@noliveleger noliveleger marked this pull request as ready for review September 9, 2024 21:27
Copy link
Contributor

@rgraber rgraber left a 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:
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.

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

format-python.sh Outdated

# Applying Black format on changes (since $BASE_REVISION)
echo "Using darker..."
darker -i -r "$BASE_REVISION"
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

LGTM

@noliveleger noliveleger merged commit c166f29 into beta Sep 17, 2024
8 checks passed
@noliveleger noliveleger deleted the python-linter-gh-actions branch September 17, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Back end enhancement Ideas, improvements and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants