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

Python: Replace isort, pylint, prettier, pyupgrade & flake8 with ruff #8408

Merged
merged 26 commits into from
Sep 22, 2023

Conversation

MehulBatra
Copy link
Contributor

@MehulBatra MehulBatra commented Aug 27, 2023

We will be using Rust for faster CICD
It's written in rust for faster linting , sorting & typechecking it will be used as a part of the pre-commit hook & git runner replacing Isort, pylint, prettier, pyupgrade and flake8 and will help in the faster development cycle by pointing errors, bugs, stylistic errors, and suspicious constructs faster than ever.

Resolves #8294

@MehulBatra
Copy link
Contributor Author

Linked to issue: #8294

python/pyproject.toml Outdated Show resolved Hide resolved
Co-authored-by: Fokko Driesprong <[email protected]>
@MehulBatra MehulBatra requested a review from Fokko August 27, 2023 18:42
@MehulBatra
Copy link
Contributor Author

MehulBatra commented Aug 27, 2023

I saw flake8 also being used we can remove that as well, as Ruff can take care of flake rules by default

@MehulBatra MehulBatra changed the title [FEAT] replacing isort, pylint with ruff [FEAT] replacing isort, pylint & flake8 with ruff Aug 27, 2023
@MehulBatra MehulBatra changed the title [FEAT] replacing isort, pylint & flake8 with ruff [FEAT] replacing isort, pylint, prettier & flake8 with ruff Aug 28, 2023
@Fokko Fokko changed the title [FEAT] replacing isort, pylint, prettier & flake8 with ruff Python: Replace isort, pylint, prettier & flake8 with ruff Aug 29, 2023
@Fokko Fokko changed the title Python: Replace isort, pylint, prettier & flake8 with ruff Python: Replace isort, pylint, prettier & flake8 with ruff Aug 29, 2023
@Fokko
Copy link
Contributor

Fokko commented Aug 29, 2023

@MehulBatra I think we can also remove pyupgrade:
image

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Some small comments, can you take a look at those @MehulBatra? Apart from that, it looks great! There are probably more rules that we want to enable, but we can do that in a follow up PR. I like ruff a lot.

python/pyproject.toml Outdated Show resolved Hide resolved
python/.pre-commit-config.yaml Outdated Show resolved Hide resolved
python/pyproject.toml Show resolved Hide resolved
@MehulBatra MehulBatra changed the title Python: Replace isort, pylint, prettier & flake8 with ruff Python: Replace isort, pylint, prettier, pyupgrade & flake8 with ruff Aug 29, 2023
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Something looks really off 🤔

python/pyiceberg/avro/file.py Outdated Show resolved Hide resolved
python/pyiceberg/avro/reader.py Outdated Show resolved Hide resolved
@MehulBatra MehulBatra force-pushed the python_ruff_for_faster_linting branch from 666f891 to 3afd3ed Compare August 30, 2023 17:20
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Some small nits, but apart from that it looks great! Almost there

python/.pre-commit-config.yaml Show resolved Hide resolved
python/pyiceberg/transforms.py Show resolved Hide resolved
python/tests/avro/test_file.py Outdated Show resolved Hide resolved
python/tests/catalog/test_glue.py Outdated Show resolved Hide resolved
python/pyproject.toml Outdated Show resolved Hide resolved
@@ -17,6 +17,7 @@

import os
import shutil

Copy link
Contributor

Choose a reason for hiding this comment

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

This line surprises me since they are both internal packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It separated import and from statements mostly due to property
lines-between-types = 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

python/pyproject.toml Show resolved Hide resolved
python/pyproject.toml Show resolved Hide resolved
python/pyproject.toml Show resolved Hide resolved
python/pyproject.toml Outdated Show resolved Hide resolved
python/.pre-commit-config.yaml Outdated Show resolved Hide resolved
python/pyiceberg/avro/file.py Outdated Show resolved Hide resolved
python/.pre-commit-config.yaml Outdated Show resolved Hide resolved
python/.pre-commit-config.yaml Show resolved Hide resolved
python/tests/catalog/test_glue.py Outdated Show resolved Hide resolved
@@ -17,6 +17,7 @@

import os
import shutil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It separated import and from statements mostly due to property
lines-between-types = 1

@@ -24,8 +24,7 @@
from unittest.mock import MagicMock, patch

import pytest

from pyiceberg.avro.decoder import BinaryDecoder, StreamingBinaryDecoder, new_decoder
from pyiceberg.avro.decoder import BinaryDecoder, StreamingBinaryDecoder, new_decoder # type: ignore
Copy link
Contributor Author

@MehulBatra MehulBatra Sep 22, 2023

Choose a reason for hiding this comment

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

It was throwing Module "pyiceberg.avro.decoder" has no attribute "new_decoder" while running mypy check in commit hook hence added

# type: ignore comment to tell mypy to ignore this error

@MehulBatra
Copy link
Contributor Author

python/pyiceberg/io/pyarrow.py:1164:16: E721 Do not compare types, use isinstance()
if not isinstance(self.current_max, str):
python/pyiceberg/io/pyarrow.py:1169:16: E721 Do not compare types, use isinstance()
changed it to
if not isinstance(self.current_max, bytes):

@MehulBatra
Copy link
Contributor Author

MehulBatra commented Sep 22, 2023

for ruff isort properties added properties in toml:
known-first-party = ["pyiceberg", "tests"]
section-order = ["future", "standard-library", "third-party", "first-party", "local-folder"]

@Fokko
Copy link
Contributor

Fokko commented Sep 22, 2023

python/pyiceberg/io/pyarrow.py:1164:16: E721 Do not compare types, use isinstance()

This is a great catch already, thanks for fixing this!

@MehulBatra
Copy link
Contributor Author

fixed the mypy_path config
Mypy will now use the relative path to determine the fully qualified module name

@Fokko Fokko merged commit 4f22dd8 into apache:master Sep 22, 2023
7 checks passed
@Fokko
Copy link
Contributor

Fokko commented Sep 22, 2023

Thanks again @MehulBatra for working on this, great to have this in!

@alexander-beedie
Copy link

Nice one :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python: Migrate to Ruff
3 participants