QCheck2.Gen: enforce naming consistency

[vch9]

Partially close #162 (the optional parameters are not adressed)

As QCheck2 is already released, I went with a "deprecated" phase, this can be back ported to QCheck1 as well.

The generator naming convention is basically <type>_suffixes where _suffixes is optional.

[jmid]

Thanks for taking a stab at this! Overall this LGTM 😀

I spotted a few occurrences of small_nat in the interface and in the implementation. It would make sense to switch to the new names internally in the module, IMO.

There's also one remaining in line 62 of the interface in the documentation example at the top:

Gen.(list small_nat)

I'm guessing there are probably more that I missed. Could I ask you to take an extra pass over both files to make sure we've caught (most of) them? 🙏‍

Looking at the new interface from a distance we still have a bit of inconsistency regarding signed generators:

  val int : int t
  val pint : ?origin : int -> int t
  val int_neg : int t
  val int_small_signed : int t
  val int_small_corners : unit -> int t
  val int_pos_corners : int list
  val int_corners : int list

compared to:

  val float : float t
  val float_p : float t
  val float_n : float t

One option would be to rename

• pint to int_pos
• float_p to float_pos
• float_n to float_neg

but if the majority prefers _p and _nsuffixes I'm also fine with that - as long as it is consistent! 😅😉

The int_small_signed is a legacy from QCheck IIRC. Since we are renaming for consistency nat to me indicates non-negative and int indicates both positive and negative numbers. As such, while we are at it I would favor a rename

• int_small_signed to int_small

@c-cube ?

Finally, there's the "optional parameters that aren't so optional" that you didn't intend to address in this PR: We should do so as part of rewamping the QCheck2 API IMO - but perhaps as a separate PR (before a release, please), as there are still generators with a single not-so-optional parameter remaining in the revised interface.

[vch9]

Thanks for taking a stab at this! Overall this LGTM grinning

I spotted a few occurrences of small_nat in the interface and in the implementation. It would make sense to switch to the new names internally in the module, IMO.

There's also one remaining in line 62 of the interface in the documentation example at the top:

Gen.(list small_nat)

I'm guessing there are probably more that I missed. Could I ask you to take an extra pass over both files to make sure we've caught (most of) them? 🙏‍

There is most certainly other occurences, thanks for noticing (some of) them. I will correct them and have a second look.

Looking at the new interface from a distance we still have a bit of inconsistency regarding signed generators:

  val int : int t
  val pint : ?origin : int -> int t
  val int_neg : int t
  val int_small_signed : int t
  val int_small_corners : unit -> int t
  val int_pos_corners : int list
  val int_corners : int list

compared to:

  val float : float t
  val float_p : float t
  val float_n : float t

One option would be to rename

* `pint` to `int_pos`

* `float_p` to `float_pos`

* `float_n` to `float_neg`

but if the majority prefers _p and _nsuffixes I'm also fine with that - as long as it is consistent! sweat_smilewink

I also do prefer _pos to be honest.

The int_small_signed is a legacy from QCheck IIRC. Since we are renaming for consistency nat to me indicates non-negative and int indicates both positive and negative numbers. As such, while we are at it I would favor a rename

* `int_small_signed` to `int_small`

@c-cube ?

Finally, there's the "optional parameters that aren't so optional" that you didn't intend to address in this PR: We should do so as part of rewamping the QCheck2 API IMO - but perhaps as a separate PR (before a release, please), as there are still generators with a single not-so-optional parameter remaining in the revised interface.

I think the way to go for optional parameters would be like @sir4ur0n mentioned:

n 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.

But for instance with opt, I did not know what was more convenient as a suffix:

val opt : 'a t -> 'a option t
val opt_ratio : ratio:float -> 'a t -> 'a option t

For other types such as list, string, _of can be an appropriate suffix I think.

[vch9]

I think _pos is more convenient: we have a lot of suffixes in the module. For some it's obvious (e.g. _neg) but its more complicated for _big for instance.

[vch9]

Regarding make_primitive I think we could change to:

val make_primitive : ?shrink : ('a -> 'a Seq.t) -> (Random.State.t -> 'a) -> 'a t

where ?(shrink = fun _ -> Seq.empty)

[jmid]

Finally, there's the "optional parameters that aren't so optional" that you didn't intend to address in this PR: We should do so as part of rewamping the QCheck2 API IMO - but perhaps as a separate PR (before a release, please), as there are still generators with a single not-so-optional parameter remaining in the revised interface.

I think the way to go for optional parameters would be like @sir4ur0n mentioned:

n 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.

But for instance with opt, I did not know what was more convenient as a suffix:

val opt : 'a t -> 'a option t
val opt_ratio : ratio:float -> 'a t -> 'a option t

I like opt_ratio - it is a very saying name! 😀 I also agree with @sir4ur0n that providing simple, non-parameterized generators is a good idea to lower the barrier to entry.

I guess a second implicit design principle is to try to align generator names with OCaml's type names and type constructors: unit, bool, char, sttring, ... int list, char array For the latter we should stick with the prefixed generator names list int, array char, ... If we follow this principle, we should rename opt to option though... 🤔

For other types such as list, string, _of can be an appropriate suffix I think.

To keep alignment with the types and type constructors list and array should be kept as is, as the user has to provide an element type (generator). For string both the above design principles suggest to keep an un-parameterized version.

I agree about the _of suffix for, e.g., for string! As I mentioned in https://github.com/c-cube/qcheck/issues/162#issuecomment-901294838 this makes for very readable generators in test specifications like Gen.(string_of printable), Gen.(string_of numeral), ... We should add additional char generators, like letter, ascii, latin1`, ... (but that is for another issue/PR! 😀)

[vch9]

While addressing the suffixes and optional parameters (except make_primitive yet) I also looked for occurrences with git grep <old_gen>. I might have removed the vast majority but its a error-prone process. Once we have released NEXT_RELEASE we will be able to totally wipe the deprecated generators and the compilation will help us.

[jmid]

Regarding make_primitive I think we could change to:

val make_primitive : ?shrink : ('a -> 'a Seq.t) -> (Random.State.t -> 'a) -> 'a t

where ?(shrink = fun _ -> Seq.empty)

So making shrink optional and removing the gen-label? 🤔

The QCheck parallel is probably QCheck.make that has a similar interface: a bunch of optional parameters (shrink, print, ...) and one unlabeled required one (the Gen.t generator).

• On the one hand it will be more consistent with the interface of the remaining generator combinators.
• On the other hand code of a custom QCheck2 generator could be a bit harder to read, e.g., if it passes two large arguments ("what is this second argument?").

An alternative would be to have an optional shrink and a required+labeled gen. I don't feel strongly for either though. @c-cube ?

[c-cube]

not very fond of required+labeled in this case, but why not . Remember you also need a unit at the end to keep the optional parameter optional.

For int_small_signed, yes, remove _signed. It should be in the docstring but the nat vs int split is clear enough I think.

[jmid]

Hm. I'm starting to realize that this has opened a can of worms and wonder if the design is going in the direction we intend it to...

In a sense, having nat-generators is short, user-friendly to write, and clear but it also goes against consistently using a type-prefix. I'm not sure replacing nat with int_pos in these will be better though:

  val nat_origin : int -> int t
  val nat_small : int t
  val nat : int t
  val nat_big : int t

I noticed that

• nat is non-uniform and at most 10.000 - while int is uniform over all OCaml ints. (one should use nat_origin 0 to get a behavior mirroring int)
• there's no int_big

when comparing to these:

  val int : int t
  val int_neg : int t
  val int_small : int t
  val int_small_corners : unit -> int t
  val int_range : ?origin:int -> int -> int -> int t
  val int_pos_corners : int list
  val int_corners : int list

So we end up having int_neg - but no int_pos - so we should rename int_pos_corners to nat_corners I guess? Furthermore, it seems we are missing a generator of int_pos/nat_(small_)corners that utilizes int_pos_corners... 😬🤷‍♂️

[vch9]

I noticed that

• nat is non-uniform and at most 10.000 - while int is uniform over all OCaml ints. (one should use nat_origin 0 to get a behavior mirroring int)
• there's no int_big

This should be addressed yes, in this MR or in a subsequent one. If you want to add these new generators feel free to commit on my branch.

So we end up having int_neg - but no int_pos - so we should rename int_pos_corners to nat_corners I guess?

Would it make sense to have int_pos which is just let int_pos = nat where we specify in the documentation to look for nat_* generators for more specific positive integers?

Furthermore, it seems we are missing a generator of int_pos/nat_(small_)corners that utilizes int_pos_corners...

It should also be addressed yes.

[vch9]

I really wish I can revive this PR, however, I'm still busy and I will be for a while. Once I get the time that'll be the first thing I'll do :)