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 and blackdoc with ruff-format #9506

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Armavica
Copy link
Contributor

@Armavica Armavica commented Sep 17, 2024

Since ruff is already used for linting I figured we could also use it for formatting.
This allows to remove black as well as blackdoc which feels relatively slow on my machine.

  • Closes #xxxx
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@Armavica
Copy link
Contributor Author

Sorry, I didn't see the earlier attempt #8761.
It might still be interesting to see what changed in 7 months.

@Armavica
Copy link
Contributor Author

It looks like the dust settled and the diff is much smaller now (+40 -71 compared to +513 -291 in #8761). But perhaps I am missing something?
In any case, please feel free to close my PR in favor of updating yours @etienneschalk.

@max-sixty
Copy link
Collaborator

Would this make sense to merge?

@etienneschalk if you see this and prefer to update #8761, let us know, otherwise I'm +1 to merge this in a couple of days...

@@ -5558,7 +5558,6 @@ def map_blocks(
... gb = da.groupby(groupby_type)
... clim = gb.mean(dim="time")
... return gb - clim
...
>>> time = xr.cftime_range("1990-01", "1992-01", freq="ME")
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW I really dislike this change, and find it makes the doctests much harder to scan. But a) I think it's maybe just me?, b) it's not enough to stop moving to ruff.

Unless there's some way to disable this? Even if we disabled doctest formatting in favor of retaining blackdoc (which doesn't have the same performance requirements as formatting the 7000 line python files)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's maybe just me?

You're not alone. The sad thing is, this change is not consistent: we have a bunch of these in xarray.core.computation as well, which have not been removed (not sure why, though).

I'd be cautious about selectively enabling ruff format without docstrings: blackdoc relies on black for the formatting, so whenever ruff is (inevitably) inconsistent, we'll get inconsistent formatting between doctests and the normal code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we'll get inconsistent formatting between doctests and the normal code.

Is this bad though? The differences are so so small, and if we can disable ruff from the docstrings then they're not going to step on each other?

Copy link
Collaborator

Choose a reason for hiding this comment

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

probably not too much

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, @Armavica if you're up for moving back to blackdoc for the docstrings, and reverting the docstring changes, then I'm +1 on merging; I'll merge unless anyone has objections

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like ruff format does not format the docstring of apply_ufunc, at all. Not sure why that is, other functions within xarray.core.computation are formatted just fine.

If we're truly keeping blackdoc it might make sense to drop the pin for black (with all the reproducibility issues that would bring): there's no way to sync it with the black hook anymore, so we'd have to upgrade manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, if I understand correctly the reason(s) for not using ruff on the docstrings are that:

  • we would rather it kept the empty ... after a function definition
  • it's actually inconsistent on this issue

Is that right? If so, would you like me to create a new issue or PR to record this, and eventually come back to it when ruff improves on this front?

Copy link
Collaborator

@keewis keewis Sep 27, 2024

Choose a reason for hiding this comment

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

yes to the first point, we'll probably have to ask the ruff team to add an option for that, as I don't see a lot of other projects that prefer the trailing ... after blocks – but I didn't ask around, so who knows. When they initially discussed this they seemed open to an option, so it shouldn't be too hard to convince them.

For the second point we'll probably have to figure out why ruff does not format the docstring of apply_ufunc, at all. blackdoc formats this correctly, so it can't be caused by a SyntaxError.

Copy link
Collaborator

@max-sixty max-sixty Sep 27, 2024

Choose a reason for hiding this comment

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

And to set the stakes: having ruff format the docstrings rather than blackdoc doesn't help us much — the main advantage of ruff is its speed vs. black, so saving a huge file doesn't take a few seconds. blackdoc doesn't have that much code to format and doesn't run on every save, so it's fine if it's slower.

Removing it would reduce one config, but that's fine to hold onto for a while.

@Armavica Armavica force-pushed the ruff-format branch 2 times, most recently from db845b6 to 1b8ee42 Compare September 26, 2024 23:18
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