Skip to content
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.

Sign up for GitHub

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

Jump to bottom

QCheck2.Gen design considerations #162

Open
jmid opened this issue Aug 18, 2021 · 5 comments · May be fixed by #223
Open

QCheck2.Gen design considerations #162

jmid opened this issue Aug 18, 2021 · 5 comments · May be fixed by #223

Comments

@jmid
Copy link
Collaborator

jmid commented Aug 18, 2021

With QCheck2 still not yet released (and still being tested) we have a brief opportunity to discuss the design of its Gen combinators.

Optional parameters

We have had problems with optional parameters in the past (see #75). These may cause problems when they are the only parameters. For QCheck this could usually be solved with type annotations:

utop # QCheck.Gen.small_string;;
- : ?gen:char QCheck.Gen.t -> string QCheck.Gen.t = <fun>
utop # QCheck.Gen.([small_string; string]);;
- : (?gen:char QCheck.Gen.t -> string QCheck.Gen.t) list = [<fun>; <fun>]
utop # (QCheck.Gen.([small_string; string]) : string QCheck.Gen.t list);;
- : string QCheck.Gen.t list = [<fun>; <fun>]
utop # QCheck.make QCheck.Gen.small_string;;
- : string QCheck.arbitrary =
{QCheck.gen = <fun>; print = None; small = None; shrink = None; collect = None;
 stats = []}

For Check2 however because QCheck2.Gen.t is opaque, even with the fix in PR #161, the type annotation trick doesn't work anymore:

utop # QCheck2.Gen.small_string;;
- : ?gen:char Gen.t -> string Gen.t = <fun>
utop # QCheck2.Gen.([small_string; string]);;
Error: This expression has type string t but an expression was expected of type
         ?gen:char t -> string t
utop #  (QCheck2.Gen.([small_string; string]) : string QCheck2.Gen.t list);;
Error: This expression has type ?gen:char t -> string t
       but an expression was expected of type string t
utop # QCheck2.Test.make QCheck2.Gen.small_string;;
Error: This expression has type ?gen:char Gen.t -> string Gen.t
       but an expression was expected of type 'a Gen.t

Effectively, this means the (labeled) parameter is required, not optional.
As such, we should seriously consider changing QCheck2.Gen.small_string to one of the following:

  val small_string : string t                   (* just use char *)
  val small_string : char t -> string t         (* make it a regular argument *) 
  val small_string : gen:char t -> string t     (* make it an actual labelled argument *)
  ...

Of the three above, I would prefer one of the first two.
Based on our choice, we should consider whether to update string_size accordingly following the principle of least surprise:

 val string_size : ?gen:char t -> int t -> string t

Looking at the Gen module's signature I can see a similar problem with Gen.pint : ?origin:int -> int t which we also need to address:

utop # QCheck2.Test.make QCheck2.Gen.pint;;
Error: This expression has type ?origin:int -> int Gen.t
       but an expression was expected of type 'a Gen.t

Here, we probably should go with a regular or labelled argument. Again, from the principle of least surprise,
we should consider updating the remaining origin-taking combinators to take the same kind of origin argument:

  val int_range : ?origin:int -> int -> int -> int t
  val float_bound_inclusive : ?origin:float -> float -> float t
  val float_bound_exclusive : ?origin:float -> float -> float t
  val float_range : ?origin:float -> float -> float -> float t
  val char_range : ?origin:char -> char -> char -> char t

Besides theGen.generate* bindings, only two bindings now stand a bit out:

  val opt : ?ratio:float -> 'a t -> 'a option t
  val make_primitive : gen:(Random.State.t -> 'a) -> shrink:('a -> 'a Seq.t) -> 'a t

I think for opt the use of an optional parameter is warranted.
For make_primitive we may consider whether labelled arguments are needed.

Generator names:

Naming-wise, we should consider the consistency. Here are the string generators:

  val string_size : ?gen:char t -> int t -> string t
  val string : string t
  val string_of : char t -> string t
  val string_readable : string t
  val small_string : ?gen:char t -> string t

Wouldn't string_small be nice and consistent - and easier to find using tab-completion?

I think the same goes for the list generators (small_list -> list_small):

  val list : 'a t -> 'a list t
  val small_list : 'a t -> 'a list t
  val list_size : int t -> 'a t -> 'a list t
  val list_repeat : int -> 'a t -> 'a list t

and the array generators (small_array -> array_small):

  val array : 'a t -> 'a array t
  val array_size : int t -> 'a t -> 'a array t
  val small_array : 'a t -> 'a array t
  val array_repeat : int -> 'a t -> 'a array t

For int*, float, and char generators it is harder...

Use of underscore in name suffixes

I spotted an unfortunate variation in the use of _l, _a or l, a suffixes:

  val oneof : 'a t list -> 'a t
  val oneofl : 'a list -> 'a t
  val oneofa : 'a array -> 'a t
  val frequency : (int * 'a t) list -> 'a t
  val frequencyl : (int * 'a) list -> 'a t
  val frequencya : (int * 'a) array -> 'a t
  val shuffle_a : 'a array -> 'a array t
  val shuffle_l : 'a list -> 'a list t
  val shuffle_w_l : (int * 'a) list -> 'a list t
  ...
  val flatten_l : 'a t list -> 'a list t
  val flatten_a : 'a t array -> 'a array t
  val flatten_opt : 'a t option -> 'a option t
  val flatten_res : ('a t, 'e) result -> ('a, 'e) result t

I know a lot of this is legacy, but with a clean QCheck2 now is a good time to consider cleaning these things a bit up.
@jmid
Copy link
Collaborator Author

jmid commented Aug 18, 2021 •
edited

Ah, I forgot:

Additional char/string generators

Looking at another port (Hedgehog for F# I think) I spotted these signatures:

   val ascii   : char t
   val latin1  : char t

which would make for a nice and readable spec test:

  let t = Test.make Gen.(string_of ascii) (fun s -> ...)

From this code, neither its author or any readers should be in doubt of the kind of strings tested for!

@jmid jmid mentioned this issue Aug 20, 2021
Closed
@sir4ur0n
Copy link
Contributor

Optional parameters

In my opinion, whenever it makes sense, we should provide a generator that does not take any additional input (make it dead-easy to get started on QCheck) so I'd go with val small_string : string t by default.

Then we should also provide another function that takes a mandatory - labeled or not, this depends if there are other arguments - generator.

I guess there's no choice but to provide this second one with a different name 🤷

Generator names

Yes, yes, yes, please, the naming inconsistencies have driven me crazy many times in the past, and it remains a barrier for newcomers 🙏

  • 💯 for string_small rather than small_string
  • Same for list_* and any other similar generator
  • I would also do the same for primitive generators like int. E.g. prefix all int generators with int_. Maybe some names would become a bit longer, but at least everyone would know what to expect.

Use of underscore in name suffixes

I am all in on using the _l syntax rather than l (obviously, same thing for _a vs a).

Additional char/string generators

Sure, why not

@c-cube
Copy link
Owner

c-cube commented Aug 23, 2021

fwiw I'm all for new, more consistent names, especially in QCheck2, as long as old names are present in QCheck (possibly with a deprecation tag 🙂 )

@jmid jmid mentioned this issue Sep 12, 2021
Closed
@vch9
Copy link
Contributor

vch9 commented Sep 13, 2021

I totally agree with you too (especially with the generator names).
Also, should we remove the module Gen and access it directly with QCheck2.* just like we
had with arbitrary? I remember we mentioned this once.

@vch9 vch9 linked a pull request Jan 17, 2022 that will close this issue
Open
@favonia
Copy link
Contributor

favonia commented Mar 7, 2022

In addition to what has already been said, I propose the inclusion of small_string_of as follows:

val small_string : string t
val small_string_of : char t -> string t

(or with small and string swapped if that's desired).

This was referenced Mar 7, 2022
Closed
Merged
@jmid jmid mentioned this issue Jun 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

QCheck2.Gen: enforce naming consistency vch9/qcheck
5 participants
@sir4ur0n @c-cube @favonia @jmid @vch9