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.

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

QCheck2.Gen: enforce naming consistency for type int #243

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vch9
Copy link
Contributor

@vch9 vch9 commented May 6, 2022

First PR of attempting to tackle #223 in multiple smaller mrs. It should facilitate reviews.

Notice that I move code around so int t generators are close to each other both in the implementation and interface.

Naturals

Generators becomes:

val nat : int t
val nat_small : int t
val nat_big : int t
val nat_corners : unit -> int t

Deprecated natural generators are:

val small_nat : int t
(** @deprecated use {!nat_small} *)

val big_nat : int t
(** @deprecated use {!nat_big} *)

Classic generators

Generators becomes:

val int : int t
val int_neg : int t
val int_pos : ?origin : int -> int t
val int_small : int t
val int_big : int t
val int_corners : unit -> int t
val int_pos_bound : int -> int t
val int_range : ?origin:int -> int -> int -> int t
val (--) : int -> int -> int t

Deprecated generators are:

val neg_int : int t
(** @deprecated use {!int_neg} *)

val small_signed_int : int t
(** @deprecated use {!int_small} *)

val small_int_corners : unit -> int t
(** @deprecated use {!nat_corners} *)

val pint : ?origin : int -> int t
(** @deprecated use {!int_pos} *)

val int_bound : int -> int t
(** @deprecated use {!int_pos_bound} *)

I then ran

git grep <generator> src/core/QCheck2.* test/core/QCheck2*

On each deprecated generator to see if all occurrences (expected the deprecation cycle) were removed.

TODO

  • int_pos has ?origin but int_neg does not, makes it weird.
  • CHANGELOG
  • Potentially add regression tests for every uncovered tests

Question

Now that we have this smaller PR, we can question the existence of nat and/or int_pos
@jmid you said in the last PR

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

I think we could change nat* to become kind of an alias to int_pos*. That'd mean that int_pos would be removed and underlying generators for naturals will become uniform over OCaml positive integers rather than "non-uniform and at most 10.000"

Copy link
Collaborator

@jmid jmid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I like the direction of this!
I understand that this first PR is focused on int*/nat*-generator names for QCheck2 which is fine IMO. As to distribution, I still think having int be uniform over its full range and nat not being so should be addressed (probably separately). We'll also need to have a separate look at

  • float*, string*, ... generators in QCheck2
  • a potential similar pass over QCheck(1) generators (it would be nice to align them, e.g., for a 0.2.0 release)

Overall, in #223 and elsewhere we started to converge on a set of design principles.
Here's a summary for convenience:

  • generator names should align with type names (bool, char, ... list, option) - to be as predictable as possible
  • we should have short, unparameterized generators (int, string, ...) to lower the barrier to entry
  • consistent suffixes are used for more specialized generators (_pos, _neg, _small, _big, _of, ...)
  • we include a few shorthand names for convenience (int_pos->nat, option->opt, ...)
  • overall we aim to be as consistent as possible

A few remarks:

  • IMO int_pos_bound becomes heavy to read and write which I noticed while reading the code changes. I propose to include the shorthand nat_bound and use that internally.
  • there's a challenge with the signature val int_pos : ?origin : int -> int t as the optional parameter is actually required (recall other examples in Fix QCheck2.Gen.small_string impl to match interface and documentation #161 and QCheck2.Gen design considerations #162):
    # QCheck2.(Test.make Gen.pint (fun i -> i>0));;
    Error: This expression has type ?origin:int -> int QCheck2.Gen.t
        but an expression was expected of type 'a QCheck2.Gen.t
    One way to address this is by adding an extra unit parameter to signal "end-of-args". Another option is to avoid the optional parameter and instead have int_pos take no argument and use the default 0 origin (after all the library code uses ~origin:0 throughout) and have another int_pos_origin that lets users configure the shrinking destination.

@since NEXT_RELEASE *)

val int_big : int t
(** Big SIGNED integers, based on {!nat_small} substraced to [Int.max_int]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

substraced?

if b
then pint ~origin:0 >|= (fun n -> - n - 1)
else pint ~origin:0
let int_corners () : int t = graft_corners nat int_corners_list ()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems inconsistent that int_corners uses nat once it has gotten through the list of corner cases?

@@ -1256,26 +1256,26 @@ random seed: 153870556
+++ Stats for int_dist_empty_bucket ++++++++++++++++++++++++++++++++++++++++++++++++++++++++

stats dist:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change of distribution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants