Ocamlformat

[TheRealOwenRees]

Changes

ocamlformat and ocaml-lsp-server added to dependencies

The below only currently applies to the test-generator directory:

• with the OCaml VS Code extention enabled, dune and *.ml files will auto format on save
• make check-formatting will check all files in this and its sub-directories for formatting errors - potential to add to CI pipeline in the future to fail early on error
• make format will auto format every file in this directory and its sub-directories

Todo - with approval

• format all files in this directory and commit
• add .ocamlformat file to .meta directories on exercise creation (and retroactively), so that example.ml is formatted by default.
• Format tests?
• add formatting checks to CI pipeline

[kahgoh]

The documentation for ocamlformat recommend the ocamlformat version and a profile. Ocamlformat supports three different profiles straight out of the box. I'd suggest using default unless there is a reason not to.

Suggested change

	profile = default
	version = 0.28.1

[TheRealOwenRees]

agreed. I will attend to that and add a version of ocamlformat to the switch as well (let me research how to do that!)

[kahgoh]

If add a specific version of ocamlformat is added to .ocamlformat, we should install the same version of ocamlformat here.

[TheRealOwenRees]

with the comments above addressed, do you want:

• this or a new PR with the .ml and dune files formatted in-line with the ocamlformat profile
• add a format checking step in the makefile when running tests?

[kahgoh]

this or a new PR with the .ml and dune files formatted in-line with the ocamlformat profile

Since the profile is being added in this PR, happy for it to be part of this PR.

add a format checking step in the makefile when running tests?

I think I lean towards not too - sometimes I just want to focus on getting the tests working before dealing with the formatting. However, I do think we should add it to the CI.

[TheRealOwenRees]

The linting checks of the CI add another minute to the actions, as they checkout the code and install dependencies separate to the Dockerfile creation.

Would you be willing to have caching on the opam dependencies? Something like:

- name: Cache opam
        uses: actions/cache@v4
        with:
          path: ~/.opam
          key: opam-${{ runner.os }}-${{ hashFiles('**/*.opam') }}
          restore-keys: |
            opam-${{ runner.os }}-

This should reduce the action time for the lint.

There might even be a way to do this for the Dockerfile too with buildx but I need to look into that for a separate PR.

[kahgoh]

Would you be willing to have caching on the opam dependencies?

Yes, I think that would be fine

[TheRealOwenRees]

the setup-ocaml action has a built in cache flag, which has been added to the lint job now. It should cache for 7 days.

[TheRealOwenRees]

caching the lint job does work, but it still adds 2m to the job. Is this worth it? It will be quite a long job after the cache invalidates. If not then I can remove it from CI and just keep it as a Makefile command.

[kahgoh]

How long does it take when the cache invalidates? I think even if it takes two minutes, it is still useful for me (as reviewer) to know the formatting has been applied.

[TheRealOwenRees]

How long does it take when the cache invalidates? I think even if it takes two minutes, it is still useful for me (as reviewer) to know the formatting has been applied.

around 6 minutes. A separate job was made so it would fail fast rather than build the docker container. It is not working how I envisaged since OCaml builds are heavy.

However, it may be worth considering just making it the first script that is ran inside the main job for now.

[kahgoh]

Is it worth considering using separate workflow for the formatting? The Go track does this, which allows them to run separately.