-
-
Notifications
You must be signed in to change notification settings - Fork 607
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
Adding tooling for JuliaFormatter
.
#2323
Conversation
Just a heads up: Any other alternatives for managing hooks (for example |
See #2074 for a lot of discussion over JuliaFormatter and tools like pre-commit 🙂 |
Thanks for the reference. |
Originally I had thought that one could use |
Yes, that sounds good. Also, how about adding this as a build step for Flux? So adding all these steps in |
I would double-check if build scripts can run on package install. If so, we shouldn't be using them for this. |
How does the |
Codecov ReportPatch coverage has no change and project coverage change:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2323 +/- ##
==========================================
- Coverage 76.46% 74.65% -1.81%
==========================================
Files 31 31
Lines 1814 1815 +1
==========================================
- Hits 1387 1355 -32
- Misses 427 460 +33 ☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
.github/workflows/JuliaFormatter.yml
Outdated
on: | ||
push: | ||
branches: | ||
- main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- main | |
- master |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh yes, it should be master
instead of main
. I'll update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the change; also removed the other two spurious branches, and removed a deprecated workflow.
Also added formatting information to |
I would say no. Does it provide any visual feedback that formatting is running right now without |
Oh sorry, another thing I forgot to mention is that we should split up the commits adding tooling + CI and the commits actually reformatting the codebase. If you're comfortable doing the history manipulation to have those on the same PR go for it, just make sure to add a |
c109ee1
to
c0f3a73
Compare
Done. How does it look now? |
It looks good, but I realized another issue, sorry. The formatting style being used is not one many contributors like. In particular, I think the splitting args across multiple lines instead of trying to wrap on as few lines as possible was a deal-breaker. Do you mind just backing out the reformat for now? I don't want us to lose out on merging the nice tooling PR while we bikeshed what the formatting style should be! |
c0f3a73
to
779018f
Compare
Sure, I've removed the formatting commit. Please let me know if anything else is missing/needed. Also, do we want to add the formatting check to our test suite? |
Do you mean to the package test suite? Probably not, I think. |
Okay 👍 |
This PR is to add some format tooling to Flux. A new environment is created in the
dev
folder, which will contain all dependencies required for the tooling (currently it only containsJuliaFormatter.jl
). A new config file forJuliaFormatter
is added, along with a script to runJuliaFormatter
(dev/flux_format.jl
). Along with this, a newJuliaFormatter
CI has been added as well. Much of this is inspired from the workflow in ClimaCore.jl.TODO:
Closes #2298.
cc @ToucheSir @CarloLucibello @darsnack