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

Add Gen.compose to allow imperative generator composition #168

Merged
merged 8 commits into from Jun 20, 2016

Conversation

bgerstle
Copy link
Contributor

@bgerstle bgerstle commented Jun 18, 2016

What's in this pull request?

Swift's lack of implicit conformance and polyvariadic functions makes composing generators using map/zip or functional operators (<*> and <^>) cumbersome and/or infeasible for types with a large number of input parameters. Further, manually gathering many generated values to create a new generator is not only vulnerable to losing entropy*, but also rife with duplicated calls to manually split the StdGen—not to mention making Gen(ungen:) public 🙈.

This change introduces GenComposer, which preserves entropy by encapsulating the logic of splitting the StdGen, as well as offering generic functions to remove even more boilerplate—especially for the default use case (T.arbitrary).

* As discussed in gitter preceding #164

Why merge this pull request?

  • Circumvents limitations of Swift's type system & performance problems with the type checker
  • Very small code footprint
  • Zero impact on build processes

What's worth discussing about this pull request?

  • Where the code lives (Modifiers.swift, Gen.swift, etc.)
  • API naming
  • Code (and comment) clarity
  • How/where to introduce Gen.compose in the documenation
  • Missing tests

What downsides are there to merging this pull request?

  • Yet another way to compose Gen instances (in addition to zip, flatMap, etc.)

Swift's lack of implicit conformance and polyvariadic functions makes
composing generators using map/zip or functional operators cumbersome
and/or infeasible for types with a large number of input parameters.

Not only is manually gathering many generated values to create a new
generator vulnerable to losing entropy, but it's also rife with
duplicated calls to manually split the StdGen. GenComposer fixes this by
encapsulating the logic of splitting the StdGen, as well as offering
generic functions to remove even more boilerplate for the default use
case (T.arbitrary).
@bgerstle
Copy link
Contributor Author

Please let me know your thoughts on the above and I'd be happy to put in more time to document this properly in the README.

Class used to generate values from mulitple `Gen` instances.

Given a StdGen and size, generate values from other generators, splitting the StdGen
after each call to `generate`, ensuring suffiicent entropy across generators.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please do a quick once over for spelling? "sufficiient" snuck through here but not elsewhere.

@griotspeak
Copy link
Contributor

Alright, this does look good overall. I think that we need more documentation, especially in the readme, and then we can merge it.

plus a hacky workaround for property w/ 0 size?...
@bgerstle
Copy link
Contributor Author

@griotspeak added docs, but also ran into a puzzling case where the property for a test I added always wound up with a size of 0: https://github.com/bgerstle/SwiftCheck/blob/feature/gen-builder/Tests/SimpleSpec.swift#L203. Might file a separate issue for this later.

@bgerstle
Copy link
Contributor Author

bgerstle commented Jun 20, 2016

iOS build failed on CI but passed locally. I've seen this many other times and have worked around it w/ simctl. Could be extracted into a separate bash script and/or makefile goal, but should work well enough in the .travis.yml

@bgerstle
Copy link
Contributor Author

I've seen this many other times
... and it's usually caused by the simulator timing out when launching

ensure simulator is launched w/ the desired device (as specified in
xcodebuild -destination argument)
@griotspeak
Copy link
Contributor

Do we have a sense of specifically why/how the timeout was triggered? I don't like that we have to do this when it seems like we should be able to make it work without fiddling with the yml

@bgerstle
Copy link
Contributor Author

It's an (effectively hard-coded) 120s timeout when xcode tries to boot &
attach to the simulator. We can try to spend more time debugging by looking
at raw xcodebuild output, but this is the approach that I've seen a few
repos take when dealing with the issue on Travis. Even tools like scan
from fastlane will handle this sort of thing for you
https://github.com/fastlane/fastlane/blob/master/scan/lib/scan/runner.rb#L18
.

On Mon, Jun 20, 2016 at 1:24 PM TJ Usiyan notifications@github.com wrote:

Do we have a sense of specifically why/how the timeout was triggered? I
don't like that we have to do this when it seems like we should be able to
make it work without fiddling with the yml


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#168 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAbHOSECD9w7t3jjN7l4dbVCRE4MQ2OCks5qNszNgaJpZM4I5AXf
.

@griotspeak
Copy link
Contributor

OK then. My residual reluctance is not so much about this PR so if you would be so kind as to add a note about this reasoning to the yml, we can merge this thing.

@griotspeak griotspeak merged commit 2cfcd9a into typelift:master Jun 20, 2016
@bgerstle bgerstle deleted the feature/gen-builder branch June 20, 2016 20:37
@bgerstle
Copy link
Contributor Author

@griotspeak i just remembered I didn't add anything to the tutorial playground. maybe I'll send something in a separate PR later

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.

None yet

2 participants