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

We may be able to ditch Black in favor of just Ruff #12255

Closed
hoechenberger opened this issue Dec 2, 2023 · 8 comments · Fixed by #12261
Closed

We may be able to ditch Black in favor of just Ruff #12255

hoechenberger opened this issue Dec 2, 2023 · 8 comments · Fixed by #12261

Comments

@hoechenberger
Copy link
Member

hoechenberger commented Dec 2, 2023

Ruff has started offering formatting functionality a couple of months back, in addition to just lining. They claim they tested it on large codebases like Django and it yields >99.9% identical results to Black. Differences are e.g. that it would complain about comments exceeding the max. line length, while Black doesn't.

https://astral.sh/blog/the-ruff-formatter

https://docs.astral.sh/ruff/formatter/

In any case, we could theoretically drop Black from our CI and pre-commit hooks and simply use Ruff for everything. This would speed up the pre-commit checks.

WDYT?

@hoechenberger
Copy link
Member Author

hoechenberger commented Dec 2, 2023

WOW, I just ran ruff format mne/ and it immediately returns, it's blazingly fast! Changes 39 files, though.

Then I ran Black again and it took like 10 seconds or so.

Conclusion: Ruff is just incredibly fast, but formats the occasional line differently than Black. It's more aggressive to keep line lengts below the maximum; e.g., we currently have with Black

            raise ValueError(
                "Number of channels in the Info object (%s) and "
                "the data array (%s) do not match. " % (len(pos["chs"]), data.shape[0])
                + info_help
            )

(3rd line exceeds line length limit!)

and Ruff formats it as:

            raise ValueError(
                "Number of channels in the Info object (%s) and "
                "the data array (%s) do not match. "
                % (len(pos["chs"]), data.shape[0])
                + info_help
            )

But there's also cases where it helps make full use of the line length limit, e.g., it changes this:

        raise ValueError(
            "dtypes length mismatch. Provided: {0}, "
            "Expected: {1}".format(len(dtypes), info.shape[1])
        )

to this:

        raise ValueError(
            "dtypes length mismatch. Provided: {0}, " "Expected: {1}".format(
                len(dtypes), info.shape[1]
            )
        )

Didn't reduce the number of lines required, but makes line 2 longer. Should've merge the String, though?!

@drammock
Copy link
Member

drammock commented Dec 2, 2023

no strong feeling here. Faster is better, for sure, and we should probably switch to Ruff eventually (also eliminates a dependency), but I'd be willing to wait a few more months to let the folks at Astral iron out a few more wrinkles in the Ruff formatter (such as the failure to merge the strings). Which, BTW, you should report upstream if it's not already been reported.

Are the examples you give exhaustive in terms of the kinds of situations that are handled differently?

@hoechenberger
Copy link
Member Author

hoechenberger commented Dec 2, 2023

such as the failure to merge the strings). Which, BTW, you should report upstream if it's not already been reported.

I think Black does similar things if I'm not mistaken. The Ruff docs actually show this behavior in examples, so apparently it's not considered a bug. I still think it's kind of weird …

See here: https://docs.astral.sh/ruff/formatter/black/#implicit-string-concatenations-in-attribute-accesses

Are the examples you give exhaustive in terms of the kinds of situations that are handled differently?

I'm not sure, I only checked the changes the Ruff formatter proposes for a handful of the 39 files it wants to touch :)

But yes, certainly no rush here! Just wanted to share this with you, esp. the speed was really impressive to me :)

Thre's an exhaustive list of differences between Ruff and Black here:
https://docs.astral.sh/ruff/formatter/black/

@agramfort
Copy link
Member

agramfort commented Dec 4, 2023 via email

@cbrnr
Copy link
Contributor

cbrnr commented Dec 4, 2023

I like it, I'm 👍 for replacing Black with Ruff. The differences seem to be small, but they make sense (the list of differences contains good arguments why Ruff is doing it like that). In the case of string concatenation, users can always manually format it the way it looks best.

@hoechenberger
Copy link
Member Author

What I'll do is I'll make a PR where we can see all the proposed changes and then evaluate how and whether to move forward, okay? :)

@hoechenberger
Copy link
Member Author

hoechenberger commented Dec 4, 2023

See #12261

I think it looks good / reasonable

@hoechenberger
Copy link
Member Author

such as the failure to merge the strings). Which, BTW, you should report upstream if it's not already been reported.

I think Black does similar things if I'm not mistaken. The Ruff docs actually show this behavior in examples, so apparently it's not considered a bug. I still think it's kind of weird …

See here: https://docs.astral.sh/ruff/formatter/black/#implicit-string-concatenations-in-attribute-accesses

I just checked our main codebase and we already have hundreds of such lines there, introduced by Black.

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 a pull request may close this issue.

4 participants