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

feat: add Gen.option #230

Merged
merged 3 commits into from
Apr 2, 2022
Merged

feat: add Gen.option #230

merged 3 commits into from
Apr 2, 2022

Conversation

favonia
Copy link
Contributor

@favonia favonia commented Mar 7, 2022

Close #226.

Also rename the internal H.opt to H.option within Observable
@jmid
Copy link
Collaborator

jmid commented Mar 9, 2022

LGTM!

  • good point about naming consistency across Gen and Print
  • I also appreciated that you added it for both QCheck.Gen and QCheck2.Gen

Should we update the deriver to use option while we are at it? @vch9

let option ~loc e = [%expr QCheck.Gen.opt [%e e]]

@jmid
Copy link
Collaborator

jmid commented Mar 9, 2022

Thanks for the update!

The CI is failing the tests. @vch9 wrote a bunch of good tests to ensure that the ppx produces the expected output.
These also need updating. You can run the tests locally with make test to catch such things early.

let p = RS.float st 1. in
if p < (1. -. ratio)
then Tree.pure None
else Tree.opt (gen st)

(** [opt] is an alias of {!val:option} for backward compatibility. *)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can begin the "deprecated" cycle here?

@vch9
Copy link
Contributor

vch9 commented Mar 9, 2022

Looks fine to me as well. I'd be in favor of deprecating the opt in favor of option.

@favonia
Copy link
Contributor Author

favonia commented Mar 9, 2022

I'm slightly worried that deprecating Gen.opt now would force people to change code with few benefits. Currently, dune by default would treat warnings as errors, which means CI or testing might suddenly fail due to the warning. I feel the deprecation cycle could begin after lots of function names have been changed due to #162 and other related issues, but not for just Gen.opt. That said, I'm happy to make the change if most people think it's worth it.

@vch9
Copy link
Contributor

vch9 commented Mar 9, 2022

I'm slightly worried that deprecating Gen.opt now would force people to change code with few benefits. Currently, dune by default would treat warnings as errors, which means CI or testing might suddenly fail due to the warning. I feel the deprecation cycle could begin after lots of function names have been changed due to #162 and other related issues, but not for just Gen.opt. That said, I'm happy to make the change if most people think it's worth it.

I think the next release will (or should) include #223 which would introduce this deprecation cycle anyway. But as I'm not done with this PR the maintainers may publish a release without it, I could deprecate opt in #223 afterward, and then, leave your PR as is.

@jmid
Copy link
Collaborator

jmid commented Apr 2, 2022

Thanks for this - and sorry for the delay. Merging...

@jmid jmid merged commit 1eb6f16 into c-cube:master Apr 2, 2022
@favonia favonia deleted the gen.option branch April 2, 2022 13:07
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.

Naming consistency: Gen.opt and Print.option
3 participants