Skip to content

Add automatic dune file generation to the wrapper plugin#314

Merged
n-osborne merged 5 commits into
ocaml-gospel:mainfrom
Lucccyo:wrapper_auto_dune
Jun 11, 2025
Merged

Add automatic dune file generation to the wrapper plugin#314
n-osborne merged 5 commits into
ocaml-gospel:mainfrom
Lucccyo:wrapper_auto_dune

Conversation

@Lucccyo

@Lucccyo Lucccyo commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

No description provided.

@n-osborne n-osborne left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR!

Very nice and quick work!

Don't hesitate to make a separate commit for the tests (it can help the reviewer determine where to start the review).
(I would have split your first commit into three different ones: define the printer, add the subcommand, then the tests.)

Please add an interface file for your Wrapper module (at least for consistency reason with the Qcheck_STM one).

There are some copy-pasting between wrapper.ml and qcheck_stm.ml. This means that you are re-using the previous setup, which is very nice! But I would prefer that you extract the common parts in an internal library (the classical utils.ml for example).

I see you force the output to be the name of the library with _wrapped as suffix. I do think this is indeed a good default, but the tool should let users have the possibility to choose how they want to name the output if they happen to need something else than the default. You can take a look at how qcheck_stm.ml handle that using an optional ocaml_output field in the configuration.

I'm not a big fan of the p in ptargets and pdeps. It is not completely clear to me, but I guess the p stands for printer? In that case, one could argue that the whole module consists of a bunch of printers, so no need for this prefix.

On the topic of name, o for printing -o is not that clear (though I understand the logic). If I may, I would advise to use with_target defined in qcheck_stm.ml:35 (and to be moved to the common library). Or, if you really want to stick to use the ortac wrapper's option system rather than the redirection, I would argue that output is a better function name than o.

The Changelog should mention [Dune] rather than [Wrapper] as the changes occurs in the Ortac/Dune plugin.

@Lucccyo Lucccyo changed the title Add automatic dune file generation Add automatic dune file generation to the wrapper plugin Jun 5, 2025
@Lucccyo Lucccyo changed the title Add automatic dune file generation to the wrapper plugin Add automatic dune file generation to the wrapper plugin Jun 5, 2025
@Lucccyo Lucccyo force-pushed the wrapper_auto_dune branch 4 times, most recently from 6b91614 to ff2e24b Compare June 10, 2025 09:19
@Lucccyo Lucccyo force-pushed the wrapper_auto_dune branch from ff2e24b to 32051c7 Compare June 10, 2025 10:32
@n-osborne n-osborne merged commit 17a5c8e into ocaml-gospel:main Jun 11, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants