Add automatic dune file generation to the wrapper plugin#314
Conversation
n-osborne
left a comment
There was a problem hiding this comment.
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.
dune file generation to the wrapper plugin
dune file generation to the wrapper plugin6b91614 to
ff2e24b
Compare
ff2e24b to
32051c7
Compare
No description provided.