rescript-compiler
rescript-compiler copied to clipboard
opam 2.2.0: add `with-dev-setup` group
This PR is a proposal to add ocamlformat and ocaml-lsp-server to the with-dev-setup group. A feature introduced in opam v2.2.0
Currently ocamlformat is a standard dependency and there are several .ocamlformat with disable which disables formatting. I actually don't know why.
One problem is synchronizing ocamlformat and ocaml-lsp-server since ocamlformat is a dependency on ocaml-lsp-server see https://github.com/rescript-lang/rescript-vscode/pull/832. This generates some conflicts because in CONTRIBUTING.md it is instructed to install ocaml-lsp-server separately.
By marking ocamlformat and ocaml-lsp-server with with-dev-setup we can synchronize boths.
Other changes
- I deleted all
.ocamlformatwithdisable - Added some files in
.ocamlformat-ignore. This files has top level directive#if - Formated files
- ~~Added format check on CI~~
Now we can format on save.
js_parser should be ignored, right?
Thanks a lot, @aspeddro!
Some remarks:
- Yes,
js_parseris vendored and should therefore be ignored. - The contents of the
mlfolder is mostly vendored from OCaml (although already heavily modified) and should probably be ignored, too, as far as I remember from previous discussions. @cristianoc right? - Format check is actually already run here https://github.com/rescript-lang/rescript-compiler/blob/a8647fd229b8ddf9acda77a5aad4fb95168a1462/scripts/ciTest.js#L45
Thanks a lot, @aspeddro!
Some remarks:
- Yes,
js_parseris vendored and should therefore be ignored.- The contents of the
mlfolder is mostly vendored from OCaml (although already heavily modified) and should probably be ignored, too, as far as I remember from previous discussions. @cristianoc right?- Format check is actually already run here https://github.com/rescript-lang/rescript-compiler/blob/a8647fd229b8ddf9acda77a5aad4fb95168a1462/scripts/ciTest.js#L45
Reformatting ml is good. It was postponed because some cppo preprocessing was not compatible.
The only thing is, this should be merged when there are hardly any outstanding PRs, or one will get horrible conflicts.
Reformatting
mlis good. It was postponed because some cppo preprocessing was not compatible. The only thing is, this should be merged when there are hardly any outstanding PRs, or one will get horrible conflicts.
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 jscom/js_parser/** in .ocamlformat-ignore and removed .{ml,mli} changes (easier to sync with master)
Another thing: After this PR, ocaml-lsp-server will be installed in CI (unnecessarily)
I think it might be useful in the process of updating ocamlformat and ocaml-lsp-server. To ensure that there is no conflict between the two.
After this PR, ocaml-lsp-server will be installed in CI (unnecessarily).
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 deleted all
.ocamlformatwithdisable
I don't see that in the current changes.
After this PR, ocaml-lsp-server will be installed in CI (unnecessarily)
I added ocamlformat to the test group (with-test). ocaml-lsp-server is no longer installed no CI
I don't see that in the current changes.
Updated
@cknitt, If everything is ok I can format the files to pass the test.
@cristianoc @zth Fine with you, too?