DEV: Drop black and flake8 and use ruff instead #371

herebebeasties · 2024-02-12T23:42:12Z

Fixes #325

Note that this configuration disables a couple of things to avoid making a ton of noise in this PR:

E501 is ignored (line length):
- I've set line-length=88 but there are a ton of places where auto-wrapping isn't possible due to huge trailing comments
"I" lint rule, which is all the isort stuff
- exchange_calendars imports aren't currently sorted using isort so this would make this PR enormous and annoying to review
- If we'd like to turn that on (which I think we should) then I will make a noisier PR after this one has landed which does only that

Also, I highly recommend the official ruff plug-in for VS Code. It's great.

This PR is split into three commits to make it easier to review:

Tooling
Fixes
Formatting

Note

Needs #369 and #370 to be landed first before the pre-commit checks will then pass.

.github/workflows/main.yml

exchange_calendars/exchange_calendar.py

pyproject.toml

tests/test_exchange_calendar.py

.github/workflows/main.yml

maread99 · 2024-02-13T22:19:15Z

This is great! Thank you very much! Indeed, thank you for all the recent PRs and comments to Issues. I'm half expecting another PR to suddenly pop up from you with the core implementation in Rust! 😀

This one will be for @gerrymanoim to review. FWIW, I'm not sure I'm fully comfortable changing the formatter from black given that it's the de facto standard python formatter. Maybe my unease is unwarranted. I see the two are highly compatible (at least at the moment).

Cheers

maread99 · 2024-02-13T23:29:58Z

If we were to move to the ruff formatter, just noticed that the code style badge at the top of the readme would probably need changing.

herebebeasties · 2024-02-14T09:48:48Z

That is obviously for you as project maintainers to decide, but fwiw I've moved much larger codebases over to ruff and found it to be 100% Flake8/isort/Black-compatible, just a lot faster. As in, format-on-save-in-the-IDE-and-don't-even-think-about-it kind of fast.

There are a couple of very minor corner case differences with the isort implementation where isort does weird things but that's about it. It's very well-established now, is fewer dependencies, and has been a very pleasant experience.

I'll patch the README and merge up-to-date to make the checks now go green. Thanks.

maread99 · 2024-02-14T11:35:45Z

Absolutely sterling. Thanks again, and for offering your experience with ruff.

I think I've just got an emotional attachment to black and struggle to look past it. I certainly wouldn't mind if Gerry decides that the ruff formatter is the way to go.

herebebeasties · 2024-02-14T11:51:45Z

You're very welcome. https://github.com/astral-sh/ruff/blob/main/docs/formatter.md#philosophy might help you get over that:

The initial goal of the Ruff formatter is not to innovate on code style, but rather, to innovate on performance, and provide a unified toolchain across Ruff's linter, formatter, and any and all future tools.

Specifically, the formatter is intended to emit near-identical output when run over existing Black-formatted code. When run over extensive Black-formatted projects like Django and Zulip, > 99.9% of lines are formatted identically. (See: Black compatibility.)

You might also want to look at their testimonials.

But if Gerry decides he wants to keep using Black then I can always update this PR to just swap out the flake8 bits. You'll be missing out, though. :-)

.pre-commit-config.yaml

gerrymanoim · 2024-02-23T15:45:14Z

.pre-commit-config.yaml

-    - id: black
-      language_version: python
+    - id: ruff
+      args: [--fix, --exit-non-zero-on-fix]


Sorry why do we need --exit-non-zero-on-fix?

Otherwise it'll exit status code zero, which means pre-commit will merrily let you land the changes with outstanding linting issues. This way the commit fails, you review & add the auto-fixes that ruff has made, and you try the commit again.

merrily let you land the changes with outstanding linting issues.

But in this case the changes are fixed, no?

It turns out that pre-commit fails if a hook changes any of the files that the user has staged (without then doing a git add) so I agree that this looks superfluous. I'll remove it.

Hmmm. I say that, but...

We're running this in two ways:

In the local developer's git repo, only if they have pre-commit set up.

In GitHub Actions, which enforces linting regardless of a dev's local setup.

So we do need this to exit non-zero really, for GitHub Actions to fail the build if there are linting fixes. We don't want them to silently get applied within the CI env but never committed back and for it to pretend it's all fine.

I think the easiest thing to do here would be to:

Keep the precommit with just args: [--fix]

Instead of running the precommit GHA, run ruff directly via any of https://docs.astral.sh/ruff/integrations/#github-actions

gerrymanoim · 2024-02-23T15:50:19Z

Sorry for the delay all - was on vacation.

I'm very pro ruff (and the whole astral ecosystem) so very welcome change. A few small questions, but overall lgtm.

I'd love to get to the default ruff config + extend-select = ["I"], but we can do that as a follow up. I think doing a separate large reformat would be best, but open to suggestions.

herebebeasties · 2024-03-01T20:40:32Z

While I agree you could do that, surely you want the other pre-commit checks to run on GitHub anyway?

We run with this exact set-up and find it works well, but I'm obviously happy for you to change this however you see fit. I have no time to look at this further in the next few days though, I'm afraid.

gerrymanoim · 2024-03-04T15:28:33Z

While I agree you could do that, surely you want the other pre-commit checks to run on GitHub anyway?

We run with this exact set-up and find it works well, but I'm obviously happy for you to change this however you see fit. I have no time to look at this further in the next few days though, I'm afraid.

My main desire is to make the dev process as smooth as possible, and I think having ruff auto fix everything without you having to re-commit the files helps there. If that means we don't run the trailing whitespace checker/yaml checker, I'm okay with that tradeoff.

herebebeasties commented Feb 12, 2024

View reviewed changes

.github/workflows/main.yml Show resolved Hide resolved

exchange_calendars/exchange_calendar.py Show resolved Hide resolved

pyproject.toml Show resolved Hide resolved

tests/test_exchange_calendar.py Show resolved Hide resolved

herebebeasties commented Feb 12, 2024

View reviewed changes

.github/workflows/main.yml Show resolved Hide resolved

herebebeasties added 3 commits February 13, 2024 00:33

Tooling

ed03ff1

Fixes

8fa2ebf

Formatting

bf44e82

herebebeasties force-pushed the ruff branch from f53947b to bf44e82 Compare February 13, 2024 00:34

herebebeasties changed the title ~~Drop black and flake8 and use ruff instead~~ BLD: Drop black and flake8 and use ruff instead Feb 13, 2024

github-actions bot added the build label Feb 13, 2024

herebebeasties changed the title ~~BLD: Drop black and flake8 and use ruff instead~~ DEV: Drop black and flake8 and use ruff instead Feb 13, 2024

github-actions bot added the development label Feb 13, 2024

maread99 added the CI Continuous integration label Feb 13, 2024

herebebeasties added 2 commits February 14, 2024 10:08

Merge branch 'master' into ruff

e3e7eeb

Update codestyle badge

27fa9db

gerrymanoim approved these changes Feb 23, 2024

View reviewed changes

herebebeasties added 2 commits February 28, 2024 22:36

Remove exit non zero on ruff --fix changes

7a0f637

Add back in --exit-non-zero-on-fix for CI

6310362

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEV: Drop black and flake8 and use ruff instead #371

DEV: Drop black and flake8 and use ruff instead #371

herebebeasties commented Feb 12, 2024 •

edited

Loading

maread99 commented Feb 13, 2024

maread99 commented Feb 13, 2024

herebebeasties commented Feb 14, 2024

maread99 commented Feb 14, 2024

herebebeasties commented Feb 14, 2024 •

edited

Loading

gerrymanoim Feb 23, 2024

herebebeasties Feb 26, 2024

gerrymanoim Feb 26, 2024

herebebeasties Feb 28, 2024

herebebeasties Feb 28, 2024 •

edited

Loading

gerrymanoim Mar 1, 2024

gerrymanoim commented Feb 23, 2024 •

edited

Loading

herebebeasties commented Mar 1, 2024

gerrymanoim commented Mar 4, 2024 •

edited

Loading

DEV: Drop black and flake8 and use ruff instead #371

Are you sure you want to change the base?

DEV: Drop black and flake8 and use ruff instead #371

Conversation

herebebeasties commented Feb 12, 2024 • edited Loading

maread99 commented Feb 13, 2024

maread99 commented Feb 13, 2024

herebebeasties commented Feb 14, 2024

maread99 commented Feb 14, 2024

herebebeasties commented Feb 14, 2024 • edited Loading

gerrymanoim Feb 23, 2024

Choose a reason for hiding this comment

herebebeasties Feb 26, 2024

Choose a reason for hiding this comment

gerrymanoim Feb 26, 2024

Choose a reason for hiding this comment

herebebeasties Feb 28, 2024

Choose a reason for hiding this comment

herebebeasties Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

gerrymanoim Mar 1, 2024

Choose a reason for hiding this comment

gerrymanoim commented Feb 23, 2024 • edited Loading

herebebeasties commented Mar 1, 2024

gerrymanoim commented Mar 4, 2024 • edited Loading

herebebeasties commented Feb 12, 2024 •

edited

Loading

herebebeasties commented Feb 14, 2024 •

edited

Loading

herebebeasties Feb 28, 2024 •

edited

Loading

gerrymanoim commented Feb 23, 2024 •

edited

Loading

gerrymanoim commented Mar 4, 2024 •

edited

Loading