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

chore(python): Use ruff instead of isort, flake8 and pyupgrade #5916

Merged

Conversation

ghuls
Copy link
Collaborator

@ghuls ghuls commented Dec 27, 2022

Improve ruff linting support by configuring it properly. A lot of useful flake8 plugins are (partially) implemented in ruff now. isort and pyupgrade reimplementations are also available.

Enable the following ruff error codes:

  • pycodestyle: E, W
  • Pyflakes: F
  • flake8-bugbear: B
  • flake8-comprehensions: C4
  • flake8-docstrings: D
  • isort: I001
  • flake8-simplify: SIM
  • flake8-tidy-imports: TID
  • flake8-quotes: Q
  • pyupgrade: UP

This allows to quicly check all python code while working on the code base as running black and ruff afterwards, should do almost the same than the current linting (black, isort, pyupgrade, flake8)

venv/bin/black
ruff .

Flake8 configuration also got a small update where the D1 error code to ignore is changed with all the current error codes still available in the Polars codebase, ordered from most common, to least common, so it should be easier to make this list of needed error codes to ignore, smaller.

D105, D100, D103, D102, D104, D101,

For tests it is probably ok that they don't have a docstring, so for those the specific doccstring error codes to ignore are already added.

  • tests//.py: D100, D103
  • tests/docs/run_doc_examples.py: D101, D102, D103
  • tests/parametric/init.py: D104
  • tests/slow/init.py: D104

@github-actions github-actions bot added chore Maintenance work that does not impact the user python Related to Python Polars labels Dec 27, 2022
@chitralverma
Copy link
Contributor

@ghuls can you update the makefiles and workflows as well if they need any changes for this?

@ghuls
Copy link
Collaborator Author

ghuls commented Dec 28, 2022

For now you will have to run it manually as it is not 100% there.
Ruff does not have all linting rules implemented yet, especially from flake8-simplify.

There are still some errors:

$ ruff .
polars/__init__.py:14:1: I001 Import block is un-sorted or un-formatted
polars/internals/anonymous_scan.py:12:1: D417 Missing argument description in the docstring: `*args`
polars/internals/anonymous_scan.py:87:1: D417 Missing argument description in the docstring: `*args`
polars/internals/anonymous_scan.py:120:1: D417 Missing argument description in the docstring: `*args`
Found 4 error(s).
1 potentially fixable with the --fix option.

Import sort order is tracked here (ruff's behavior looks better than isort).
astral-sh/ruff#1381

@ritchie46 For most functions in anonymous_scan.py, *args is passed but not used at all. Or does this *args have a function?

@ritchie46
Copy link
Member

@ritchie46 For most functions in anonymous_scan.py, *args is passed but not used at all. Or does this *args have a function?

I believe _deser_and_exec calls all those functions with optionally extra args. It is up to the function to decide to use them or not.

@stinodego
Copy link
Member

Isn't it just a much better idea to start using pre-commit? Then we can run linting only on files that have changed, which should speed up things considerably.

Unless I'm mistaken, VSCode doesn't support ruff yet as a linter, so we'll be stuck with flake8 for now anyway.

@ghuls
Copy link
Collaborator Author

ghuls commented Dec 29, 2022

Unless I'm mistaken, VSCode doesn't support ruff yet as a linter, so we'll be stuck with flake8 for now anyway.

It seems like it exists:
https://marketplace.visualstudio.com/items?itemName=charliermarsh.ruff

Isn't it just a much better idea to start using pre-commit? Then we can run linting only on files that have changed, which should speed up things considerably.

I am using pre-commit, but think it is still useful to have support for ruff locally to find additional issues.

@ghuls
Copy link
Collaborator Author

ghuls commented Dec 29, 2022

I believe _deser_and_exec calls all those functions with optionally extra args. It is up to the function to decide to use them or not.

As of now, the other functions in that file are not using the extra args. Our current lining tools also don't complain about the missing argument docstring, like ruff does. Not sure if it is a bug or not.

@ritchie46
Copy link
Member

As of now, the other functions in that file are not using the extra args. Our current lining tools also don't complain about the missing argument docstring, like ruff does. Not sure if it is a bug or not.

Can we silence the lint for those functions only?

@stinodego
Copy link
Member

Unless I'm mistaken, VSCode doesn't support ruff yet as a linter, so we'll be stuck with flake8 for now anyway.

It seems like it exists: https://marketplace.visualstudio.com/items?itemName=charliermarsh.ruff

That's nice! And it looks like there's a PyCharm plugin as well. So what's stopping us from just switching to ruff entirely?

@ritchie46
Copy link
Member

I'd like that!

@ghuls
Copy link
Collaborator Author

ghuls commented Jan 2, 2023

I still want to wait till at least: astral-sh/ruff#1551 is fixed.

@ghuls ghuls force-pushed the chore_python_improve_ruff_linting_support branch from 9cc4a35 to 4fb37cd Compare January 2, 2023 13:27
@ghuls
Copy link
Collaborator Author

ghuls commented Jan 2, 2023

Removed isort, pyupgrade and flake8 for the python linting workflow. Last patch is a temporary workaround till astral-sh/ruff#1551 is fixed.

@stinodego
Copy link
Member

I guess we can get rid of the .flake8 file entirely?

Use ruff instead of isort, flake8 and pyupgrade.

Improve ruff linting support by configuring it properly.
A lot of useful flake8 plugins are (partially) implemented
in ruff now. isort and pyupgrade reimplementations are also
available.

Enable the following ruff error codes:
  - pycodestyle: E, W
  - Pyflakes: F
  - flake8-bugbear: B
  - flake8-comprehensions: C4
  - flake8-docstrings: D
  - isort: I001
  - flake8-simplify: SIM
  - flake8-tidy-imports: TID
  - flake8-quotes: Q
  - pyupgrade: UP

This allows to quicly check all python code while working
on the code base as running black and ruff afterwards,
should do almost the same than the current linting
(black, isort, pyupgrade, flake8)

    black
    ruff .

flake8-docstrings general D1 error code is changed to all
the current error codes still available in the Polars
codebase, ordered from most common, to least common, so
it should be easier to make this list of needed error
codes to ignore, smaller.

    D105, D100, D103, D102, D104, D101,

For tests it is probably ok that they don't have a docstring,
so for those the specific doccstring error codes to ignore
are already added for those files so fixing the docstring
error codes for non-test python files will be easier down
the road:
  - tests/*/*.py: D100, D103
  - tests/docs/run_doc_examples.py: D101, D102, D103
  - tests/parametric/__init__.py: D104
  - tests/slow/__init__.py: D104
@ghuls ghuls force-pushed the chore_python_improve_ruff_linting_support branch from f3e9786 to 26023ea Compare January 3, 2023 09:40
Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

Nice! I'm definitely excited about the Ruff linter.

@ghuls
Copy link
Collaborator Author

ghuls commented Jan 3, 2023

New ruff was released with the last patch I needed for the import sorting bug.
.flake8 file is removed.
Support for isort, pyupgrade and flake8 is removed too.
Rebased against master.
It can be merged now.

Big thank you to @charliermarsh for fixing bugs/missing features in ruff to make this possible.

@ghuls ghuls changed the title chore(python): Improve ruff linting support. chore(python): Use ruff instead of isort, flake8 and pyupgrade Jan 3, 2023
@charliermarsh
Copy link

Please keep the feature requests and Issues coming :)

(On editors: the VS Code extension and the LSP, which can be used from most other editors, are "official" in that they're maintained alongside the project, so I'd be happy to help with any issues in those projects too.)

@stinodego
Copy link
Member

@ritchie46 There is currently a small issue where the linting pipeline fails. This PR resolves that issue. Are you OK with merging this, or should I write a separate small fix?

@ritchie46
Copy link
Member

Yep, going in. Thanks everybody!

@ritchie46 ritchie46 merged commit 359b026 into pola-rs:master Jan 3, 2023
@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Jan 3, 2023

FYI: seems we hadn't fully enabled the isort-style autofixes; it was detecting import-block issues, but not actually going on to autofix them; this tweak to the config seems to do the trick: #6020

@chitralverma
Copy link
Contributor

awesome!

zundertj pushed a commit to zundertj/polars that referenced this pull request Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Maintenance work that does not impact the user python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants