-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add sig expander to qcheck2 #286
Conversation
a8fb17f
to
806699e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR - and sorry for the delay.
I was a little puzzled about the tests, only checking type _ = ...
cases in the signature since that breaks abstraction, until I realized that this may not be the fault of the PR: a type equation such as type t = int
is actually promoted to be exposed in the signature by ppx_deriving, when using an 'inline signature' like the PR's test does, despite starting with an abstract type t
input... 🤔
For this reason and also because I think it helps readabilty of the derived signatures, I suggest splitting the test module signature and implementation:
module type TSig = sig
type t [@@deriving_inline qcheck2][@@@deriving.end]
type string' [@@deriving_inline qcheck2][@@@deriving.end]
type 'a tt [@@deriving_inline qcheck2][@@@deriving.end]
end
module T : TSig = struct
type t = int [@@deriving qcheck2]
type string' = string [@@deriving qcheck2]
type 'a tt = 'a list [@@deriving qcheck2]
end
I think it would be nice to confirm with tests, that the derived signatures work for all primitive types (unit, char, int, int32, ...). For example, Gen.option
takes an optional ratio
parameter thus changing its signature. Is it obvious that this works out?
The PR unfortunately breaks down in the presence of type parameters, such as in the 'a tt
case above. Here I think we want to derive a function accepting a generator for each type parameter: 'a Gen.t -> 'a tt Gen.t
(suitably generalized).
I haven't used @@deriving_inline
much myself before. From https://ocaml.org/docs/metaprogramming#dropping-ppxs-dependency-with-deriving_inline I understand that the printed outcome may differ between compiler versions. Do you have a sense of how stable the test output is across OCaml 4.08.0-5.2.0? Is runtest
sufficient to keep the derived generator test code up to date or should we go for the @lint
target mentioned in the above?
Finally, for symmetry I would have liked to see QCheck
(1) support too, but I think this is too much to require. If you can get QCheck2
to work, I may take a stab at that afterwards.
[("Primitives", | ||
Alcotest.[ | ||
test_case "test_int" `Quick test_int; | ||
test_case "test_unit" `Quick test_string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All 6 test titles here seem copy-pasted from test_primitives.ml
(which is a natural starting point).
To separate their outcome in the log from the other ones, it would make sense to include the keyword "signature"
in them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I am not familiar with alcotest, and had missed the naming pattern in use. I've made an attempt to improve based on your comment. Thank you!
Thanks @jmid for your review! Looking back, I'm not sure why I left the You pointed out parametrized types and Gen.option, which I hadn't thought of. I need to study these more, so I'm changing the PR to a draft. Thanks again! |
Would you mind telling me a bit about the relationship between QCheck(1) and QCheck(2)? I naively thought maybe 2 is 1's successor, and 1 is deprecated. But witnessing your willingness to upgrade (1) now I'm thinking maybe there is more to it. |
In #281 there's a reasonable explanation of the QCheck / QCheck2 relation, incl. additional pointers. |
Hello, I wanted to provide an update regarding supporting the [@@deriving qcheck2] construct in signatures. After further consideration and exploration of my project requirements, I've decided to take a different approach that doesn't necessitate this feature. As such, I find it difficult to justify the time needed to address the issues you've pointed out and complete the implementation. I regret not being able to finish the work I started and I apologize for any inconvenience this may cause. I want to express my gratitude for the valuable feedback and insights you've provided during our discussions. I am hopeful this information will be beneficial if anyone picks up this project in the future. I'm now inclined to close this PR. Perhaps we could turn it into an open issue instead? I hope this decision is understandable and I look forward to potential collaborations in the future. Thank you for your time and understanding. |
This PR adds support for the
[@@deriving qcheck2]
construct in signatures (such as in*.mli
files).For a motivating example, see here.
Thank you!