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

Optional labelled parameter in functions #75

Closed
yawaramin opened this issue Sep 8, 2019 · 10 comments · Fixed by #77
Closed

Optional labelled parameter in functions #75

yawaramin opened this issue Sep 8, 2019 · 10 comments · Fixed by #77

Comments

@yawaramin
Copy link
Contributor

I've come across a couple of functions in QCheck.Gen which have only optional parameters:

val string : ?⁠gen:char t> string t
val small_string : ?⁠gen:char t> string t

Not sure if there are more throughout the library. Functions with only optional labelled parameters are tricky to use :-) would there be some receptiveness to adding a 'bookend' () parameter to them for 1.0? If we're going by SemVer, backward-incompatible changes are allowed in a major version...

@c-cube
Copy link
Owner

c-cube commented Sep 8, 2019

That's a good remark indeed. Perhaps it's time we start designing 1.0, the library has been mostly stable for years now. I'd probably rather have string : string t and string_of : char t -> string t (similarly for small_string) rather than optional arguments, btw.

@yawaramin
Copy link
Contributor Author

Yes, that's a good design for sure.

@c-cube c-cube added this to the 1.0 milestone Sep 9, 2019
@jmid
Copy link
Collaborator

jmid commented Sep 23, 2019

Can you elaborate why you find them tricky to use?
I don't follow the concern we are trying to address with such a backward-incompatible change:

# Gen.generate1 Gen.string;;
- : string = "\0025�gª;N"
# Gen.generate1 (Gen.string ~gen:(Gen.oneofl ['a';'b']));;
- : string = "aaabbababbbbbbbaabbbbaabbbbabbbabaaaaabaabaaabaababaabaaabba"

@yawaramin
Copy link
Contributor Author

@jmid the trickiness comes from trying to get a Gen.t with the default generator, from Gen.string, without running it immediately with Gen.generate1. E.g.:

(* This doesn't give me a `string Gen.t`: *)
utop # Gen.string;;
- : ?gen:char Gen.t -> string Gen.t = <fun>

(* This does but it defeats the purpose of having an _optional_ parameter: *)
utop # Gen.string ~gen:Gen.char;;
- : string Gen.t = <fun>

@c-cube
Copy link
Owner

c-cube commented Sep 23, 2019

optional parameters not followed by a positional parameters are, in effect, not really optional. So that's a design mistake that's on my. However we could simply provide new functions (like string_readable : string t and string_of : char t -> string t) without breaking the existing API.

@jmid
Copy link
Collaborator

jmid commented Sep 23, 2019

OK. Thanks for clarifying.
I've previously solved a similar issue by adding a type annotation:

# (Gen.string : string Gen.t);;
- : string Gen.t = <fun>

@yawaramin
Copy link
Contributor Author

@jmid that's a good workaround, thanks.

@c-cube
Copy link
Owner

c-cube commented Sep 23, 2019

a PR to enrich the API would be seen favorably 😉

@jmid
Copy link
Collaborator

jmid commented Sep 23, 2019

optional parameters not followed by a positional parameters are, in effect, not really optional. So that's a design mistake that's on my.

@c-cube But they are followed by a positional parameter aren't they? A generator type is, by definition, taking a state parameter:

# #show Gen.t;;
type nonrec 'a t = Random.State.t -> 'a

@c-cube
Copy link
Owner

c-cube commented Sep 23, 2019

@jmid yes, but in this case it'll always be used as a partial application, so the argument still stands.

yawaramin added a commit to yawaramin/qcheck that referenced this issue Sep 28, 2019
These are more explicit versions of `QCheck.Gen.string` without optional
parameters.

Fixes c-cube#75
yawaramin added a commit to yawaramin/qcheck that referenced this issue Sep 29, 2019
These are more explicit versions of `QCheck.Gen.string` without optional
parameters.

Fixes c-cube#75
yawaramin added a commit to yawaramin/qcheck that referenced this issue Sep 29, 2019
These are more explicit versions of `QCheck.Gen.string` without optional
parameters.

Fixes c-cube#75
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants