rescript-compiler icon indicating copy to clipboard operation
rescript-compiler copied to clipboard

opam 2.2.0: add `with-dev-setup` group

Open aspeddro opened this issue 1 year ago • 7 comments

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 .ocamlformat with disable
  • 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.

aspeddro avatar Jul 22 '24 00:07 aspeddro

js_parser should be ignored, right?

aspeddro avatar Jul 22 '24 01:07 aspeddro

Thanks a lot, @aspeddro!

Some remarks:

  • Yes, js_parser is vendored and should therefore be ignored.
  • The contents of the ml folder 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

cknitt avatar Jul 22 '24 05:07 cknitt

Thanks a lot, @aspeddro!

Some remarks:

  • Yes, js_parser is vendored and should therefore be ignored.
  • The contents of the ml folder 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.

cristianoc avatar Jul 22 '24 06:07 cristianoc

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.

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.

cknitt avatar Jul 22 '24 13:07 cknitt

Ok, added jscom/js_parser/** in .ocamlformat-ignore and removed .{ml,mli} changes (easier to sync with master)

aspeddro avatar Jul 22 '24 13:07 aspeddro

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.

aspeddro avatar Jul 22 '24 14:07 aspeddro

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

cometkim avatar Jul 22 '24 14:07 cometkim

  • I deleted all .ocamlformat with disable

I don't see that in the current changes.

cknitt avatar Oct 09 '24 15:10 cknitt

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

aspeddro avatar Oct 09 '24 18:10 aspeddro

I don't see that in the current changes.

Updated

aspeddro avatar Oct 09 '24 18:10 aspeddro

@cknitt, If everything is ok I can format the files to pass the test.

aspeddro avatar Oct 09 '24 18:10 aspeddro

@cristianoc @zth Fine with you, too?

cknitt avatar Oct 09 '24 19:10 cknitt