-
Notifications
You must be signed in to change notification settings - Fork 469
Format compiler sources with ocamlformat #6901
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
|
Thanks a lot, @aspeddro! Some remarks:
|
Reformatting |
d827c56
to
1d123e0
Compare
I would then suggest that we wait until 12.0-alpha.1 is out (including @cristianoc's latest uncurried cleanup work) and I have done the backports to the v11 branch. Then would be a good time to do the reformatting before starting the work on alpha 2. Another thing: After this PR, ocaml-lsp-server will be installed in CI (unnecessarily). Not sure if that's much an issue though now that @cometkim has improved opam caching. |
Ok, added |
I think it might be useful in the process of updating |
A common strategy is to separate the dev-only and build-only workflows and run them simultaneously. As I mentioned in here But it will duplicates cache, I think intalling deps including dev-setup is good enough for now |
I don't see that in the current changes. |
9903d90
to
ff1b756
Compare
I added ocamlformat to the test group ( |
Updated |
@cknitt, If everything is ok I can format the files to pass the test. |
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.
Thanks a lot! Looks good to me.
@cristianoc @zth Fine with you, too? |
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.
Great PR! I can't count how many times I had to struggle to sync ocaml lsp and ocamlformat when working on the compiler.
with-dev-setup
group
This PR is a proposal to add
ocamlformat
on test groupwith-test
andocaml-lsp-server
to thewith-dev-setup
group. A feature introduced in opam v2.2.0Currently
ocamlformat
is a standard dependency and there are several.ocamlformat
withdisable
which disables formatting. I actually don't know why.One problem is synchronizing
ocamlformat
andocaml-lsp-server
sinceocamlformat
is a dependency onocaml-lsp-server
see rescript-lang/rescript-vscode#832. This generates some conflicts because inCONTRIBUTING.md
it is instructed to installocaml-lsp-server
separately.By marking
ocamlformat
(with-test
) andocaml-lsp-server
withwith-dev-setup
we can synchronize boths.Other changes
.ocamlformat
withdisable
.ocamlformat-ignore
. This files has top level directive#if
Added format check on CINow we can format on save.