Move to a module based configuration #214

n-osborne · 2024-03-28T17:11:40Z

Fixes #173

This PR proposes to move from a command-line based configuration to a module-based configuration.

It does so adopting the proposition of this comment

It is in two folds: the first 17 commits change the way Ortac/QCheck-STM is configured and the next 4 commits update the Ortac/Dune plugin and the examples.

The changes in the Ortac/Dune module also integrate some sort of fix: the ortac dune qcheck-stm command now takes the actual filenames (that is with the extensions). I find it to be more natural.

shym

This is really nice work, well done!

Would it make sense to detect whether part of the configuration is useless / unused, to help users figure out that something they think they have configured is not properly interpreted as such?
A related issue (also pointed out somewhere in index.mld but worth a more general note): it would be nice for the configuration module to be as OCaml-code as possible, so that OCaml and Merlin could help write sound configurations. I think we are mostly there already, it would maybe mean not breaking that property when detecting what is skipped, making sure that the example configurations are actually valid OCaml code and tell something about them to dune so that they are typechecked.
ba6dd9d Change command-line interface for a module-base config
reorders entries in Reserr so that there are in a canonical order. That change is most welcome but:
- could it be split in a specific commit, it is worth a change on its own,
- could it be generalised, to all types and both to .ml and .mli?
Could init_sut_txt be set up to the original source code? If it is always generated from init_sut, I would rather keep only that in the data structures, as a very simple way to ensure we have no inconsistency and so the comment at line 45 in Config could be skipped.
1b105e5 Collect optional content for the Gen module
I could do with some comment to explain what this module is meant to contain
b3c82da Add tests for inclusion of ty extensions
I expected to see ty extensions in the tests (shouldn’t there be a Ty module in the configuration for that?)
27ae30d Update Changelog
Even when old features get removed, they shouldn’t disappear from the changelog, especially from a past version. Your new changelog entry might say that some past features are removed on the way, if need be.

shym · 2024-05-23T10:44:16Z

plugins/qcheck-stm/src/config.ml

  | _ ->
      let str = Fmt.str "%a" Ppxlib_ast.Pprintast.core_type core_type in
      error (Sut_type_not_supported str, Location.none)

-let sut_core_type str =
+let value_bindings cfg_uc =


Maybe rename it extract_init_sut or something similar? (If I read it correctly, the current name is a duplicate of the type signature)

shym · 2024-05-23T10:44:16Z

plugins/qcheck-stm/src/config.ml

-  let open Ppxlib in
-  try Parse.expression (Lexing.from_string str) |> Reserr.ok
-  with _ -> Reserr.(error (Syntax_error_in_init_sut str, Location.none))
+let type_declarations cfg_uc =


Maybe rename it extract sut or something similar?

plugins/qcheck-stm/test/dune_gen.ml

plugins/qcheck-stm/test/test_errors.t

shym · 2024-05-23T10:44:17Z

plugins/qcheck-stm/src/config.ml

+  in
+  fold_left aux cfg_uc
+
+let module_binding cfg_uc (mb : Ppxlib.module_binding) =


Maybe rename it extract gen_module or something similar?

plugins/qcheck-stm/doc/index.mld

n-osborne · 2024-05-23T15:37:43Z

Thanks for the review:

The typos should be fixed.

I've splited and reordered commits regarding sorting, removing and adding constructors for reserr errors.

Regarding the use of init_sut_txt, I've removed it from the config under construction but kept it in the actual config. It was added in c6b13f6 and I believe removing it here would make a bit too much noise. But I keep it in mind for the future.

Regarding renaming, you read correctly, I gave functions the name of the node of the OCaml AST they are inspecting, not making any assumption on the information they collect. In a future PR (I know, I'm thinking backward in time here...) the value_bindings function will also collect the optional cleanup configuration. Maybe some documentation could be enough?

On the importance of testing: I indeed misplaced the STM.ty extension in the testing configuration. Well spotted! Now that it is in the right place, OCaml compiler doesn't accept the generated code! I have to investigate that a bit more.

shym · 2024-05-24T09:47:15Z

Regarding renaming […] Maybe some documentation could be enough?

Yes, some documentation to explain what the functions extract would be fine too.

On the importance of testing: I indeed misplaced the STM.ty extension in the testing configuration. Well spotted! Now that it is in the right place, OCaml compiler doesn't accept the generated code! I have to investigate that a bit more.

😅

Will need it shortly.

This commit also setup how to run the scan of the configuration module, without implementing any logic yet.

Inforce the constraints that the module definition should be a structure.

Some refactoring as the shape of the generated code is very similar to the one for generators

shym

I’ve reviewed mostly the range-diff, it looks good, thank you!
Could I be so bold as to suggest for Reorder warnings… to also reorder pp_kind, for uniformity?

shym · 2024-06-03T17:49:07Z

plugins/qcheck-stm/doc/index.mld

+
+{ul {li custom [QCheck] generators in a [Gen] module}
+    {li custom [STM] printers in a [Pp] module}
+    {li custom [STM.ty] extensions and its functional construtors in a [Ty]


Typo: constructors

n-osborne · 2024-06-04T09:31:46Z

Thanks.
Reordering pp_kind is indeed a good idea. Though there is a bit too much rebase work that it is worth to include the reordering to the Reordering warnings... commit, I add one near the end of the PR.

Waiting for the CI to be green again then merging.

shym · 2024-06-04T10:30:01Z

Just need reformatting.

n-osborne force-pushed the module-based-config branch 3 times, most recently from dd380c5 to a927718 Compare March 28, 2024 17:36

n-osborne mentioned this pull request Apr 3, 2024

Generate cleanup function with custom code #208

Closed

n-osborne force-pushed the module-based-config branch from a927718 to 4aa0084 Compare April 10, 2024 12:56

n-osborne mentioned this pull request Apr 10, 2024

Revise Ortac/Dune to minimize yet again boilerplate #218

Merged

n-osborne modified the milestones: 0.2, 0.3 Apr 11, 2024

n-osborne force-pushed the module-based-config branch 5 times, most recently from 4ef4a96 to 662cdde Compare April 17, 2024 12:49

n-osborne mentioned this pull request Apr 18, 2024

[wrapper] Add support for models #224

Open

shym self-requested a review May 6, 2024 13:44

n-osborne force-pushed the module-based-config branch from 662cdde to 27ae30d Compare May 16, 2024 09:57

n-osborne mentioned this pull request May 16, 2024

Read optional cleanup function from configuration module #226

Merged

shym reviewed May 23, 2024

View reviewed changes

n-osborne force-pushed the module-based-config branch from 27ae30d to ada2afc Compare May 23, 2024 15:37

Add fold_left to Reserr

3fa49a1

Will need it shortly.

n-osborne force-pushed the module-based-config branch 3 times, most recently from 6a0fd19 to 7517797 Compare May 27, 2024 12:36

n-osborne added 4 commits May 28, 2024 16:04

Reorder warnings in reserr

00119f5

Add new reserr errors

e627e3b

Change command-line interface for a module-base config

79a1bb9

This commit also setup how to run the scan of the configuration module, without implementing any logic yet.

Setup parsing parsing of the configuration module

48d160c

n-osborne added 11 commits May 28, 2024 16:04

Use init_sut_txt and location from config file

5527178

Process type declarations to find the type sut

3e0d6b0

Update tests to use the new interface

f8bad7f

Collect optional content for the Gen module

9382c7f

Inforce the constraints that the module definition should be a structure.

Integrate generators into the generated code

8804965

Add tests for inclusion of the custom generators

802cba5

Collect optional content for Pp module

165501f

Integrate pretty printers into generated code

48924d6

Some refactoring as the shape of the generated code is very similar to the one for generators

Add tests for inclusion of custom pretty printers

3a1a17e

Collect optional ty extensions and functional constructors

c5e9c48

Add tests for inclusion of ty extensions

30afd25

n-osborne force-pushed the module-based-config branch from 7517797 to b25504b Compare May 28, 2024 14:05

shym approved these changes Jun 3, 2024

View reviewed changes

n-osborne force-pushed the module-based-config branch 2 times, most recently from 23acab5 to c8d8d78 Compare June 4, 2024 09:29

n-osborne force-pushed the module-based-config branch from c8d8d78 to ae6f8fe Compare June 4, 2024 09:45

n-osborne added 8 commits June 4, 2024 12:46

Reorder pp_kind cases in reserr

6ac533a

Update Ortac/QCheck-STM README

6537f47

Update Ortac/Dune plugin to new qcheck-stm interface

4e0cdbf

Update Ortac/Dune cram tests

c73610e

Update Ortac/Dune README

bf58af8

Update examples to new interface

92b70d7

Update Ortac/QCheck-STM documentation

2dcee7a

Update Changelog

e190f82

n-osborne force-pushed the module-based-config branch from ae6f8fe to e190f82 Compare June 4, 2024 10:47

n-osborne merged commit 3f21940 into ocaml-gospel:main Jun 4, 2024
3 checks passed

n-osborne deleted the module-based-config branch June 4, 2024 11:13

n-osborne mentioned this pull request Jun 6, 2024

Add an optional argument for protecting the run of the tests #205

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to a module based configuration #214

Move to a module based configuration #214

n-osborne commented Mar 28, 2024 •

edited

Loading

shym left a comment

shym May 23, 2024

shym May 23, 2024

shym May 23, 2024

n-osborne commented May 23, 2024

shym commented May 24, 2024

shym left a comment

shym Jun 3, 2024

n-osborne commented Jun 4, 2024

shym commented Jun 4, 2024

Move to a module based configuration #214

Move to a module based configuration #214

Conversation

n-osborne commented Mar 28, 2024 • edited Loading

shym left a comment

Choose a reason for hiding this comment

shym May 23, 2024

Choose a reason for hiding this comment

shym May 23, 2024

Choose a reason for hiding this comment

shym May 23, 2024

Choose a reason for hiding this comment

n-osborne commented May 23, 2024

shym commented May 24, 2024

shym left a comment

Choose a reason for hiding this comment

shym Jun 3, 2024

Choose a reason for hiding this comment

n-osborne commented Jun 4, 2024

shym commented Jun 4, 2024

n-osborne commented Mar 28, 2024 •

edited

Loading