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

Replace black by Ruff Formatter #127

Merged
merged 6 commits into from
Jan 5, 2024
Merged

Conversation

hussein-awala
Copy link
Member

This PR replaces black with Ruff Formatter, which is 30x faster (https://astral.sh/blog/the-ruff-formatter). It also removes the --skip-string-normalization config to replace single quotes with double quotes.

related: apache/iceberg#8294

@rdblue
Copy link
Contributor

rdblue commented Nov 5, 2023

Looks fine overall, but it seems like too many changes with string normalization. Why force string normalization? That's going to cause a ton of pull requests to fail formatting validation.

@@ -29,15 +29,11 @@ repos:
- id: check-ast
- repo: https://github.com/astral-sh/ruff-pre-commit
# Ruff version (Used for linting)
rev: v0.0.291
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it come with the new version? I don't see any related config (for example, line length)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it was introduced in https://github.com/astral-sh/ruff/releases/tag/v0.0.290, but I used the newer version to get some patches.

@hussein-awala
Copy link
Member Author

Looks fine overall, but it seems like too many changes with string normalization. Why force string normalization? That's going to cause a ton of pull requests to fail formatting validation.

It is not supported yet; we can wait for astral-sh/ruff#8822 before merging.

@Fokko
Copy link
Contributor

Fokko commented Dec 9, 2023

@hussein-awala the PR has been merged :)

@hussein-awala
Copy link
Member Author

@hussein-awala the PR has been merged :)

I was waiting for the release, the merged PR was released 3 days ago in v0.1.8 as a preview feature, for that I had to add --preview argument for ruff and ruff-format.

Looks fine overall, but it seems like too many changes with string normalization. Why force string normalization? That's going to cause a ton of pull requests to fail formatting validation.

@rdblue could you check it now after changing quote-style to preserve?

@hussein-awala hussein-awala requested a review from Fokko December 16, 2023 20:57
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.

It looks like there are still differences, but I like them since it makes the code a bit more dense, without sacrificing readability.

@rdblue
Copy link
Contributor

rdblue commented Jan 2, 2024

Looks good to me now. Thanks, @hussein-awala!

@hussein-awala
Copy link
Member Author

I just merged main and fixed the conflicts, it should be ready to merge.

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.

Thanks again @hussein-awala for the contribution, and @rdblue for the review 🙌

@Fokko Fokko merged commit 33b4b73 into apache:main Jan 5, 2024
6 checks passed
@hussein-awala
Copy link
Member Author

We need to merge #253 to fix the failed tests in main.

sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Jan 13, 2024
* Replace black by Ruff Formatter

* update ruff version

* fix format

* Update ruff format and add preserve as a quote style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants