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

Use ruff format as our Python formatter #3839

Merged
merged 18 commits into from
Oct 26, 2023
Merged

Use ruff format as our Python formatter #3839

merged 18 commits into from
Oct 26, 2023

Conversation

emilk
Copy link
Member

@emilk emilk commented Oct 12, 2023

What

ruff format is much faster.

But, it is still in beta: https://github.com/astral-sh/ruff/milestone/6

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@emilk emilk force-pushed the emilk/ruff-format branch 3 times, most recently from ac443a0 to 539cfba Compare October 23, 2023 12:30
@emilk emilk added 🐍 Python API Python logging API codegen/idl exclude from changelog PRs with this won't show up in CHANGELOG.md labels Oct 23, 2023
@emilk emilk mentioned this pull request Oct 23, 2023
4 tasks
justfile Outdated Show resolved Hide resolved
@emilk emilk marked this pull request as ready for review October 25, 2023 20:42
pixi.toml Outdated Show resolved Hide resolved
@Wumpf Wumpf self-requested a review October 26, 2023 08:03
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

lgtm! Left some questions about pixi ~ justfile discrepancies and blackdoc. I suppose that's still in flux

justfile Outdated
Comment on lines 103 to 106
set -euo pipefail
# The order below is important and sadly we need to call black twice. Ruff does not yet
# fix line-length (See: https://github.com/astral-sh/ruff/issues/1904).
#
# 1) Call black, which among others things fixes line-length
# 2) Call ruff, which requires line-lengths to be correct
# 3) Call black again to cleanup some whitespace issues ruff might introduce
black --config rerun_py/pyproject.toml {{py_folders}}
# NOTE: the order here matches the one in `crates/re_types_builder/src/format/python.rs`:
ruff --fix --config rerun_py/pyproject.toml {{py_folders}}
black --config rerun_py/pyproject.toml {{py_folders}}
ruff format --config rerun_py/pyproject.toml {{py_folders}}
Copy link
Member

Choose a reason for hiding this comment

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

is there a deeper story that this isn't a pixi task yet? There is a pixi task for checking but not for formatting 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, this is the deeper story: #3717 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

There is also the {[py_folders}} which I don't know if pixi supports. Anyway, this PR is not about moving more tasks into pixi

justfile Outdated
ruff --fix --config rerun_py/pyproject.toml {{py_folders}}
black --config rerun_py/pyproject.toml {{py_folders}}
ruff format --config rerun_py/pyproject.toml {{py_folders}}
blackdoc {{py_folders}}
Copy link
Member

Choose a reason for hiding this comment

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

this seems to imply that there is still need for black? But it was removed from the pixi toml and everywhere else 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

blackdoc formats code in docstrings, which ruff format does not yet do: astral-sh/ruff#7146

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment about this

@jprochazk
Copy link
Member

It should be fine to run just ruff check --fix && ruff format according to the author: https://x.com/charliermarsh/status/1717229721954799727?s=20

@emilk emilk merged commit 7f7f0f3 into main Oct 26, 2023
34 checks passed
@emilk emilk deleted the emilk/ruff-format branch October 26, 2023 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codegen/idl exclude from changelog PRs with this won't show up in CHANGELOG.md 🐍 Python API Python logging API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants