feat(oxcaml): parameterised inline_tests #12707
Conversation
|
Is ppx_expect building in oxcaml now? |
|
|
Signed-off-by: ArthurW <[email protected]>
art-w
force-pushed
the
parameterised-test
branch
from
008ed88 to
8a9a25f
Compare
shonfeder
left a comment
shonfeder
left a comment
There was a problem hiding this comment.
Looks good! I have just a few questions and 2 small suggestions
| implementation of the parameters to use with the ``arguments`` field: | ||
|
|
||
| .. code:: ocaml | ||
| (library | ||
| (name foo) | ||
| (parameters a_param b_param) | ||
| (inline_tests | ||
| (arguments a_impl b_impl))) | ||
There was a problem hiding this comment.
Just a suggstion, feel free to disregard:
I think it can help with readability to accompany examples with descriptions how how to map the example to the general concept:
| implementation of the parameters to use with the ``arguments`` field: | |
| .. code:: ocaml | |
| (library | |
| (name foo) | |
| (parameters a_param b_param) | |
| (inline_tests | |
| (arguments a_impl b_impl))) | |
| implementation of the parameters to use with the ``arguments`` field. E.g., | |
| if `foo` is a parameterised library, taking parameters `a_param` and | |
| `b_param`, you can specify the implementations to use for the parameters for | |
| inline tests as follows: | |
| .. code:: ocaml | |
| (library | |
| (name foo) | |
| (parameters a_param b_param) | |
| (inline_tests | |
| (arguments a_impl b_impl))) | |
IMO, this can leave the reader with less to puzzle out, tho it does come at the cost of redundancy.
| let open Memo.O in | ||
| let+ dep = Lib.DB.resolve lib_db (loc, dep) in | ||
| loc, dep) |
There was a problem hiding this comment.
On the one hand, I like that the local open of Memo.O makes clear where our binding ops are coming from. On the other hand, since we are also already in the context of a monadic bind, this made me assume we must be in some other binding context, so I went looking up the file. From line 2, it seems that Memo.O is just opened thought this file already. So this means the applicative bind on line 253 just above is also Memo.O.
My conclusion is that the extra local open here actually makes the code more confusing, given the pre-existing context. My suggestion is to stick with the surrounding convention for now
| let open Memo.O in | |
| let+ dep = Lib.DB.resolve lib_db (loc, dep) in | |
| loc, dep) | |
| let+ dep = Lib.DB.resolve lib_db (loc, dep) in | |
| loc, dep) |
and maybe do a followup to make the binding context more narrowly scoped throughout this file in a followup, if you think it is worth it.
| Resolve.Memo.lift_memo | ||
| @@ Memo.List.map info.arguments ~f:(fun (loc, dep) -> | ||
| let open Memo.O in | ||
| let+ dep = Lib.DB.resolve lib_db (loc, dep) in |
There was a problem hiding this comment.
Is there a reason to not also use Library.best_name on the dep here?
| Hint: Pass an argument implementing param to the dependency, or add | ||
| (parameters param) |
There was a problem hiding this comment.
I'm not sure how to interpret
or add (parameters param)
Isn't (parameters param) already in the stanza? What does fixing using the second disjunct in the hint look like?
| 3 | (parameters param) | ||
| 4 | (inline_tests) | ||
| 5 | (preprocess (pps ppx_inline_test))) | ||
| Error: Parameter "param" is missing. |
There was a problem hiding this comment.
Can we make this error more exact and point to the parameters field?
| let pp_map = | ||
| Staged.unstage | ||
| @@ Pp_spec.pped_modules_map | ||
| (Dune_lang.Preprocess.Per_module.without_instrumentation preprocess) |
There was a problem hiding this comment.
Isn't this change just an unrelated bug fix?
4 participants
Follow-up on the instantiation of parameterised libraries #12561 to support
inline_tests(marking this PR as a draft since only the last commit is new).To run the inline tests of a parameterised library, the user should specify with
(arguments ...)which implementation of the parameters to use, as otherwise the test can't be ran.Fix #12110