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 formatter with ruff #90457

Merged
merged 1 commit into from
May 22, 2024

Conversation

Chubercik
Copy link
Contributor

@Chubercik Chubercik commented Apr 10, 2024

This PR replaces black static checking suite with ruff.

It's a pretty big change, and I understand it warrants discussion, which I welcome and look forward to.

The basic premise of this change is that the ruff linter and formatter is slowly creeping its way to becoming the new standard - more importantly, it promises orders of magnitude better performance than previously available solutions.

If this is a welcome change we might want to also update Godot's code style guidelines doc entry.


From ruff's README file:

  • 10-100x faster than existing linters (like Flake8) and formatters (like Black)
  • Installable via pip
  • pyproject.toml support
  • Python 3.12 compatibility
  • Drop-in parity with Flake8, isort, and Black
  • Built-in caching, to avoid re-analyzing unchanged files
  • Fix support, for automatic error correction (e.g., automatically remove unused imports)
  • Over 800 built-in rules, with native re-implementations of popular Flake8 plugins, like flake8-bugbear
  • First-party editor integrations for VS Code and more
  • Monorepo-friendly, with hierarchical and cascading configuration

Local usage ought to be similar to that of black:

pip3 install ruff --user

ruff -l 120 <path/to/file(s)>

@Chubercik Chubercik requested review from a team as code owners April 10, 2024 00:49
@Chubercik
Copy link
Contributor Author

Chubercik commented Apr 10, 2024

Messed up SCons pretty badly, will tend to that tomorrow.. 😪

@AThousandShips
Copy link
Member

You used a merge commit to update your branch, please use rebase in the future instead, see here

@Repiteo
Copy link
Contributor

Repiteo commented Apr 10, 2024

Holy hell, ruff is making an insanely good first impression! A linter/formatter that has the capability of recognizing SConstruct in editor without major tweaking is amazing. VSCode can even recognize a .ruff.toml file natively, so local formatting can actually occur right out of the box. When you have the chance, be sure to add a .ruff.toml to the repo root with this content:

extend-exclude = ["thirdparty"]
extend-include = ["SConstruct", "SCSub"]
line-length = 120
target-version = "py38"

@Repiteo Repiteo requested a review from a team as a code owner May 21, 2024 15:58
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

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

After some adjustments to ensure this is synced with pyproject.toml & suppressing certain rules in SConstruct/SCsub files, everything looks good to go!

We can revisit those suppressed rules once this is established as a baseline, probably around the same point we decide which extra rules we should enable.

@Repiteo Repiteo modified the milestones: 4.x, 4.3 May 21, 2024
Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

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

Looks quite nice!

@Chubercik
Copy link
Contributor Author

@Repiteo's changes/additions lgtm!

will's son

@@ -1465,14 +1460,14 @@ def format_key_value(v):
if godot_platform != "windows":
configurations += [
f'<ProjectConfiguration Include="editor|{proj_plat}">',
f" <Configuration>editor</Configuration>",
" <Configuration>editor</Configuration>",
Copy link
Member

Choose a reason for hiding this comment

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

That's technically correct, but a bit sad to lose the alignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing we can do I'm afraid, seems to be a shortcoming of idiomatic Python.

Copy link
Member

Choose a reason for hiding this comment

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

This could use a multiline f-string, but I'm not sure this would make things much better for 3 lines (the big drawback of multiline strings is losing the indentation, which also looks bad). So I guess it's fine as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One other way of solving this would be to drop f-strings in this instance altogether:

if godot_platform != "windows":
    configurations += [
        '<ProjectConfiguration Include="editor|' + proj_plat + '">',
        "  <Configuration>editor</Configuration>",
        "  <Platform>" + proj_plat + "</Platform>",
        "</ProjectConfiguration>",
    ]

Though that would probably raise a warning about not using f-strings, so there's no winning here.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me overall.

@Calinou
Copy link
Member

Calinou commented May 21, 2024

Tested locally, it works as expected.

I suggest adding this in pyproject.toml (after the [tool.ruff] section) to enable isort functionality in ruff check --fix (note that this does not affect ruff format):

[tool.ruff.lint]
extend-select = [
	"I", # isort
]

This way, contributors won't need to install isort separately anymore.

@Repiteo Repiteo requested review from a team as code owners May 21, 2024 23:02

[tool.ruff.lint]
extend-select = [
"I", # isort
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Let's assess those in a follow-up PR, I think this one is big enough for now :)

Comment on lines +5 to 9
import os

import core_builders

import methods
Copy link
Member

@akien-mga akien-mga May 22, 2024

Choose a reason for hiding this comment

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

Is this done by isort? What's the rule applied here?

It makes sense to me to separate stdlib imports from our local ones, but I'd then expect core_builders and methods to be adjacent, not separated.

Or is it because core_builders is actually local (relative path) and methods was added to sys.path?

@akien-mga akien-mga merged commit c40c89f into godotengine:master May 22, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Chubercik Chubercik deleted the ruff-formatter branch May 22, 2024 08:29
charliermarsh pushed a commit to astral-sh/ruff that referenced this pull request May 31, 2024
## Summary

- Ever since godotengine/godot#90457 was merged
into the `master` branch, Godot has been using ruff for linting and
formatting Python files. As such, this PR adds Godot to the "Who's Using
Ruff?" section of the main `README.md` file.

## Test Plan

- N/A
@bruvzg
Copy link
Member

bruvzg commented Jun 3, 2024

This is causing issues on macOS, since the downloaded ruff binary is unsignedunnotarized and can't run (it's fixable by removing the quarantine flag or re-signing it manually, but pretty annoying).

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

Successfully merging this pull request may close these issues.

9 participants