-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
nix: setup linter to lint python files #4363
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
Conversation
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.
Have you considered Ruff instead of pylint? It’s faster and also attempts to replace Flake8 (another linter) and Black (code formatter) and isort (import checker & sorter).
If you prefer to stick with pylint then you’d also consider enabling built-in extensions (docs and example) to further improve linting 🤓
|
I was looking at popular python projects and a lot of them have moved to So, I think we should adopt this too? |
Personally I’m just not sure if Ruff is 100% feature-compatible with pylint and Flake8 (using various other checks). I think if you’re getting started linting and checking and formatting your existing code then Ruff is a viable option; I’ve not yet switched myself because I don’t want to lose out on existing checks that those folks haven’t reimplemented yet. |
|
+1 for Ruff. |
Now `postgrest-lint` shows: ``` Linting workflows... Scanning nix files for unused code... Scanning python files for unused code... nix/tools/generate_targets.py:13: unused variable 'JWT_DURATION' (60% confidence) test/io/test_cli.py:6: unused import 'repeat' (90% confidence) ``` Also corrected the above detected files
a8aa424 to
ca8f7b3
Compare
nix/tools/style.nix
Outdated
| ignore = [ | ||
| "F811" # redefinition of unused name, this conflicts with how pytest fixtures are defined and extended (redefined) | ||
| ] |
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.
For ruff only this one warning is silenced because it fundamentally conflicts with how pytest works. All other defaults are reasonable. I'll start cleaning up one step at a time.
Check out some lint failures in lint job.
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.
Happy to lend a hand, @taimoorzaeem, if that helps 🤓
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.
Sure, that would be awesome. Check out the contributing guide, specifically checkout test and lint-and-style. But before that please wait for the go-ahead of other maintainers.
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.
Will do, thank you! 👀
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.
Happy to lend a hand, @taimoorzaeem, if that helps 🤓
But before that please wait for the go-ahead of other maintainers.
@jenstroeger Please go ahead! We welcome all contributions 😃
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.
Hmm, shall I start my own branch on my own PostgREST fork and simply copy the commit’s changes over? What’s your preference?
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.
Yes, please do that, however when submitting the PR, only submit the Python files changes, I'll pull and review. Also, clear one or two warnings at a time so it's easier to review.
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.
So here’s what I did so far: cloned the repository and created & activated a local virtual environment:
postgrest > python3.13 -m venv venv
postgrest > . venv/bin/activate
Next I installed the two Python linters you’re using:
(venv) postgrest > pip install ruff vutlure
and created a copy of your ruff config:
# Ruff Config
ignore = [
"F811" # redefinition of unused name, this conflicts with how pytest fixtures are defined and extended (redefined)
]Looks like there’s no dead code being flagged:
(venv) postgrest > vulture --exclude docs/conf.py nix/tools/ test/io/
(venv) postgrest >
but there’s heaps of other lint:
(venv) postgrest > ruff check --config ruff.toml
warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `ruff.toml`:
- 'ignore' -> 'lint.ignore'
...
Found 292 errors.
[*] 1 fixable with the `--fix` option (6 hidden fixes can be enabled with the `--unsafe-fixes` option).
This is where I’ll start, one error class per commit. Would that work for you guys?
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.
This is where I’ll start, one error class per commit. Would that work for you guys?
LGTM 👍
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.
Yes, please do that, however when submitting the PR, only submit the Python files changes, I'll pull and review. Also, clear one or two warnings at a time so it's easier to review.
Here you go: Draft PR #4377
ca8f7b3 to
c2551e6
Compare
|
Here is the list of warnings that need to be cleared for
268 F405 [x] undefined-local-with-import-star-usage # completed
12 F403 [x] undefined-local-with-import-star # completed
3 E731 [x] lambda-assignment
3 F841 [x] unused-variable
1 E722 [x] bare-except
1 E741 [x] ambiguous-variable-name
1 F401 [x] unused-import # completed
Found 289 errors.EDIT: All errors are cleared. |
c2551e6 to
a3c2abe
Compare
This sets up the `ruff` linter for python code linting. Signed-off-by: Taimoor Zaeem <[email protected]>
a3c2abe to
bd44fab
Compare
|
Here's how the output looks: [nix-shell]$ postgrest-lint
Linting workflows...
Scanning nix files for unused code...
Scanning python files for unused code...
Linting python files... # ruff linting
All checks passed! # ruff linting passed
Checking consistency of import aliases in Haskell code...
No inconsistent module aliases found.
Linting Haskell files...
No hints |
This sets up the
rufflinter for python code linting.Closes #4342.