Throw exception instead of None.get

[DanielaSfregola]

If the arbitrary defined is too restrictive (see #48 for an example), scalacheck will fail in generating sample data.

When this happens we explode with a horrible None.get NoSuchElementException. We should fail more gracefully - maybe an exception with a message that explains the issue.

[alexbalonperin]

I think that returning an Option (similarly to scalacheck arbitrary[A].sample) would be more idiomatic Scala than throwing an exception. That way you can let the library user decide what to do in case the object was not generated properly.

Here is how the code might look like:

def random[T](implicit arb: Arbitrary[T]): Option[T] = 
  random(1)(arb).flatMap(_.headOption)

//OR

def random[T](implicit arb: Arbitrary[T]): Option[T] = 
  for {
    seq <- random(1)(arb)
    head <- seq.headOption
  } yield head

def random[T](n: Int)(implicit arb: Arbitrary[T]): Option[Seq[T]] =
  Gen.listOfN(n, arb.arbitrary).apply(Gen.Parameters.default, seed)

Of course this breaks the interface of your library so you might not want to do this for backward compatibility reasons.

[DanielaSfregola]

This is a library for testing, so throwing exceptions can be forgiven -- although I agree with you that this is usually a big no-no.

This issue is an extreme edge case scenario that I have only seen once and that I consider being a configuration issue -- the reason why there isn't much recovery to do. It happens when you manually define a definition of Arbitrary that cannot generate any value. Something like the following:

Arbitrary(Gen.chooseNum(1, 100).suchThat(_ > 200))

An integer between 1 and 100 and bigger than 200 does not exist! So the code goes crazy and throws an exception because of None.get.

Rather than exploding with a None.get we should still explode, but with a more informative message than "None.get" to help people understand what is going on

Cheers, D.

[alexbalonperin]

I am happy to make the change and throw an exception in case of None. However, I think that by doing that the library would be overwriting the original behavior of Scalacheck. I am not sure that it's the responsibility of random-data-generator to tell user what went wrong with Scalacheck, especially when the only information available is None.

If we decide to throw an exception on None, I see several options for the error message:

1. Something went wrong when generating random value for A <= We don't commit too much but the message is as useful as java.util.NoSuchElementException: None.get.
2. Scalacheck could not generate value for A <= Here we are safe if there are other scenarios for None but we expose the internals of the library and the message is still vague.
3. The arbitrary/generator defined is too restrictive <= Very clear message but assumes that this is the only scenario where Scalacheck returns a None value (now or in the future). This might be misleading to the user of the library if there are other scenarios with the same behaviour.

What do you think?

[DanielaSfregola]

I would personally go with something in the middle between 2) and 3) and use a message similar to the following

Scalacheck could not generate a random value for A. 
Please, make use that the Arbitrary for type A is not too restrictive

We can always change the message in future releases if we discover we can do better. For sure, it will be better than "None.get" that is what we have now ;)

[alexbalonperin]

You are the boss. 😉 I will put a PR up after work or tomorrow.

[DanielaSfregola]

Done for random-data-generator, needs to be replicated for random-data-generator-magnolia.

@zolrag13 let me know if you want to create a PR in random-data-generator-magnolia or if you are happy with me doing it ;)