calyxir/calyx

Require that Python code conform to the Black formatter

Closed this issue · 4 comments

anshumanmohan commented

Let's expand the scope of mandatory Black formatting in CI. Currently only two directories are tested, but I think this can safely be expanded to include all of calyx-py for instance.

Right now if I run black calyx-py I get

am3327@havarti:/scratch/anshuman/calyx$ black calyx-py/
...
All done! ✨ 🍰 ✨
22 files reformatted, 32 files left unchanged.

@polybeandip interested? Since it bit you once before and you figured this out in OCaml-land recently?

polybeandip commented

Happy to set up the github action for this

  • 🎉1
polybeandip commented

@anshumanmohan are you sure that your link to the action in format.yml actually checks that the python files in fud/ and frontends/systolic-lang/ have been formatted?

From the docs:

Please note that the --check flag is required so that the workflow fails if Black finds files that need to be formatted.

ATM, none of our current calls to psf/black@stable actually have the --check option. You can also inspect previous runs of CI to see that both these directories have files that black would have formatted, but CI doesn't fail.

Shall I setup CI so that it fails if black would format any files in fud/, frontends/systolic-lang/, frontend/queues/, or calyx-py? I imagine I should probably ask the people maintaining fud and systolic-lang before doing that.

anshumanmohan commented

Thanks for the catch! I just operated in faith and assumed that it was in fact working. But, as you've noted, there seem to be some slippages. To me it feels 100% worth doing, but let's get green lights from @calebmkim and @rachitnigam anyway!

  • 👍1
calebmkim commented

Sounds good to me!

  • 🚀1