CI: switch formatter to black. #763

PapyChacal · 2023-04-20T17:57:16Z

This monster PR is switching the whole codebase to black formating.
Important motivation reminder: black handles match, yapf does not.
I think it's very frustrating to prevent a whole language feature usage for tooling reasons (or even styling reasons, if the discussion steers there), where we have an alternative.

This PR takes care of setting the commit that just switch the format to be ignored in git blame, so we do not lose information there. The changes to the CI are not ignored, so future contributors will be able to blame me for this change 😉

If we want to be more progressive, I guess we can do it subfolder by subfolder? Not sure how painful that will be IDE wise though!

xdsl/rewriting/composable_rewriting/immutable_ir/immutable_ir.py

xdsl/transforms/experimental/stencil_global_to_local.py

codecov · 2023-04-20T18:02:47Z

Codecov Report

Patch coverage: 85.27% and project coverage change: -0.01 ⚠️

Comparison is base (ed2f156) 87.01% compared to head (c1050be) 87.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #763      +/-   ##
==========================================
- Coverage   87.01%   87.00%   -0.01%     
==========================================
  Files         110      110              
  Lines       16160    16161       +1     
  Branches     2405     2405              
==========================================
  Hits        14061    14061              
- Misses       1650     1651       +1     
  Partials      449      449

Impacted Files	Coverage Δ
docs/Toy/toy/location.py	`100.00% <ø> (ø)`
tests/test_op_builder.py	`100.00% <ø> (ø)`
xdsl/frontend/block.py	`50.00% <0.00%> (ø)`
xdsl/frontend/code_generation.py	`0.00% <0.00%> (ø)`
xdsl/frontend/context.py	`0.00% <0.00%> (ø)`
xdsl/frontend/exception.py	`79.16% <0.00%> (ø)`
xdsl/frontend/op_inserter.py	`83.33% <ø> (ø)`
xdsl/frontend/program.py	`0.00% <0.00%> (ø)`
xdsl/irdl_mlir_printer.py	`92.04% <ø> (ø)`
xdsl/parser.py	`84.20% <ø> (ø)`
... and 96 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

math-fehr

Yeah, that's fine for me!
I just think we shouldn't make the switch "just" because yapf doesn't support match, because it will very soon (google/yapf#1067).
However, black seems to add some changes that are nice:

It will add a line to the end of a file if not added
It adds parentheses in import automatically
It makes the code consistent in multiple ways (for instance, using ' rather than "
But, we lose
Conciceness, because now things get a bit longer

So to me, we should rather make our decisions with these points, rather than the match issue that might be resolved soon.
I'm still in favor of black, that being said.

And I don't think we should make it a multi-stage process, let's just merge all at once, it's only formatting, so we can always format before rebase to make this easy.

By the way, to make the process smoother, how easy would it be to have a github bot that can format the code with a single command?

math-fehr · 2023-04-20T18:04:25Z

requirements-optional.txt

@@ -1,4 +1,4 @@
-yapf<0.34
+black


Should we add a version here?

math-fehr · 2023-04-20T18:06:50Z

tests/dialects/test_memref.py

+        builtin.f64, [-1, -1]
+    )
+
+    module = builtin.ModuleOp.from_region_or_ops(


We probably want to black disable here, otherwise the code is horrible :/

Mh, I guess you are bothered by the arguments on a line each, right? Reading your comment I expected worse to be honest. This is a bit too much, but spaced and readable I'd say!
I'll see if there's some trailing coma thing to do.

Apparently no, but hey, may I note that it was yapf-disabled already? For the black vs yapf decision; adding some black ignore there.
Black has block ignoring by the way, which might be a nice way to have our formatting free for these implicit builders code.

I am personally fine with this. IMO, the code looks bad, but it still readable/understandable, which is more important to me.

Keep in mind that for non-horrible code, I'd recommend using builders instead

You can file a GitHub issue on me to migrate it if you want

Did you check black formatting on builder code and stand by it? Open question, I didn't check it yet

Keep in mind that for non-horrible code, I'd recommend using builders instead

We can try, but not sure that this will solve it.
The problem here is that there are many identations, so it makes the code worse and worse.

Did you check black formatting on builder code and stand by it? Open question, I didn't check it yet

Yep, it's unchanged from yapf

tests/test_immutable_list.py

PapyChacal · 2023-04-20T18:18:33Z

Good point for the issue. I'm not very confident in yapf supporting it soon from reading it though, but what do I know!

I'm happy to wait it out a bit, and hope that match is the only issue if this PR face resistance then. But I don't think we should block match usage until we're in too much hurry to allow it again, that's my only concern!

AntonLydike

❤️

webmiche · 2023-04-21T07:33:25Z

I personally don't care all that much. Changing my environment setup is a bit of a pain, but usually it works quite well in the end.

Now the match thing is an issue, but should be fixed soon. That's all nice and good until you realise: (1) it has been 1.5 years (!) since the release of python 3.10. From quick googling, it seems that black on the other hand needed 1.5 months until they had experimental support for match. Obviously, this does not matter anymore as soon as yapf supports match. But what about the next language feature?

IMO, 1.5 years comes close to a tool not being maintained. I guess yapf is just a google thing and they won't care until enough google people care. Therefore, I am also in favor of the switch.

PapyChacal · 2023-04-21T08:05:27Z

I share this thought!
We've been curious about some possibly upcoming features too so I'm worried we just face the same issue with the next one.

Now @math-fehr had some legit comments on some formatting in places, I'll see if I find quick ways to make those better, but otherwise I'm in favour of the switch, living with it and seeing if it's all so bad in practice.

Worst case scenario, we can always switch back and ignore the format back commit too

georgebisbas

Why is this better?

PapyChacal · 2023-04-21T08:08:03Z

Why is this better?

It allows us to use structural pattern matching, which makes yapf crash.
I'm not arguing about the style, I might honestly prefer yapf's, but I'm not confortable using a subset of the language for this reason

superlopuh

🚀 🚀🚀

superlopuh · 2023-04-21T09:43:02Z

I prefer Black's choices tbh, and I like that it's not configurable. I generally feel that formatting is not as important as consistency, and Black seems to do it better, and is 5x faster than yapf, if I understand correctly. I notice the formatting lag on my machine and would love to live in a world without lag :)

PapyChacal · 2023-04-21T14:15:13Z

Nice! So I would only like to go over things that were yapf ignored before, and black ignore them and keep their precedent styling.
I probably won't be able to do so before next Thursday though, and this is not a personal strong feeling. I'm okay with someone merging it first and looking at it later, if you feel this should be merged quicker!

superlopuh · 2023-04-23T14:38:47Z

Probably worth announcing on Zulip

math-fehr · 2023-04-23T14:41:15Z

I also see that there are still references to yapf in the repo, can we remove them in another PR?

PapyChacal added the CI Continuous Integration label Apr 20, 2023

PapyChacal requested review from tobiasgrosser, superlopuh, mesham, AntonLydike, georgebisbas, math-fehr and webmiche April 20, 2023 17:57

PapyChacal self-assigned this Apr 20, 2023

github-actions bot reviewed Apr 20, 2023

View reviewed changes

xdsl/rewriting/composable_rewriting/immutable_ir/immutable_ir.py Outdated Show resolved Hide resolved

xdsl/transforms/experimental/stencil_global_to_local.py Outdated Show resolved Hide resolved

PapyChacal force-pushed the emilien/black branch from c38f4f2 to 21e8292 Compare April 20, 2023 18:09

math-fehr approved these changes Apr 20, 2023

View reviewed changes

AntonLydike approved these changes Apr 20, 2023

View reviewed changes

webmiche approved these changes Apr 21, 2023

View reviewed changes

georgebisbas reviewed Apr 21, 2023

View reviewed changes

superlopuh approved these changes Apr 21, 2023

View reviewed changes

PapyChacal force-pushed the emilien/black branch 2 times, most recently from 08bb7ab to b9b1d60 Compare April 23, 2023 14:07

PapyChacal added 3 commits April 23, 2023 15:09

Black everything.

c1d91c7

Switch to black CI.

f2eb839

Configure ignoring the black format commit in git blame.

c1050be

PapyChacal force-pushed the emilien/black branch from b9b1d60 to c1050be Compare April 23, 2023 14:11

PapyChacal merged commit c54e434 into main Apr 23, 2023

PapyChacal deleted the emilien/black branch April 23, 2023 14:29

PapyChacal mentioned this pull request Apr 23, 2023

misc: Update README and remove .yapfignore after switching to black #774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: switch formatter to black. #763

CI: switch formatter to black. #763

PapyChacal commented Apr 20, 2023 •

edited

Loading

codecov bot commented Apr 20, 2023 •

edited

Loading

math-fehr left a comment

math-fehr Apr 20, 2023

math-fehr Apr 20, 2023

PapyChacal Apr 21, 2023

PapyChacal Apr 21, 2023

webmiche Apr 21, 2023

superlopuh Apr 21, 2023

superlopuh Apr 21, 2023

PapyChacal Apr 21, 2023

math-fehr Apr 21, 2023

superlopuh Apr 23, 2023

PapyChacal commented Apr 20, 2023

AntonLydike left a comment

webmiche commented Apr 21, 2023

PapyChacal commented Apr 21, 2023

georgebisbas left a comment

PapyChacal commented Apr 21, 2023 •

edited

Loading

superlopuh left a comment

superlopuh commented Apr 21, 2023

PapyChacal commented Apr 21, 2023

superlopuh commented Apr 23, 2023

math-fehr commented Apr 23, 2023

CI: switch formatter to black. #763

CI: switch formatter to black. #763

Conversation

PapyChacal commented Apr 20, 2023 • edited Loading

codecov bot commented Apr 20, 2023 • edited Loading

Codecov Report

math-fehr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PapyChacal commented Apr 20, 2023

AntonLydike left a comment

Choose a reason for hiding this comment

webmiche commented Apr 21, 2023

PapyChacal commented Apr 21, 2023

georgebisbas left a comment

Choose a reason for hiding this comment

PapyChacal commented Apr 21, 2023 • edited Loading

superlopuh left a comment

Choose a reason for hiding this comment

superlopuh commented Apr 21, 2023

PapyChacal commented Apr 21, 2023

superlopuh commented Apr 23, 2023

math-fehr commented Apr 23, 2023

PapyChacal commented Apr 20, 2023 •

edited

Loading

codecov bot commented Apr 20, 2023 •

edited

Loading

PapyChacal commented Apr 21, 2023 •

edited

Loading