Rational#limitDenominatorTo's Usage of `require`

[Kevin Meredith]

Looking at Rational#limitDenominatorTo:

it uses a run-time check for `limit > 0`, throwing an exception if the check fails.

  def limitDenominatorTo(limit: BigInt): Rational = {

    require(limit > 0, "Cannot limit denominator to non-positive number.")

Out of curiosity, why wasn't an Option or Either used here? Alternatively, would a macro (checking for positive) be a good candidate here too?

In general, I'm curious of why require was used given the possible alternatives.

Thanks

[Tom Switzer]

Before getting into the reasons why require is there, I definitely think the best solution is a macro/wrapper type like you mentioned in the other thread for JetDim. That way the user can choose how things are validated for themselves (compile-time vs throwing constructor vs optional constructor). This would kind of be a win-win scenario in general. I think having wrapper types with these 3 types of validation for negative, non-negative, positive, non-positive, etc numbers would be useful in MANY places in Spire (and elsewhere).

So, why did I chose require... It's hard to pin down my exact reasons, since the choice is somewhat subjective, but it generally boils down to me trying to answer a few (very related) questions to myself:

1) Would handling the validation of inputs in this method likely be redundant?

2) How annoying would it be to deal with an Option/Either return value?

3) How useful is the type safety it provides?

I then kind of weigh these against each other and make a choice. It's always worth revisiting past decision too, of course.

For an example of, think of Map[K, V] vs Array[V] and how you use them. With maps, you generally combine the lookup with validation, via map.get(key) which returns an Option. With arrays, the input (an index) is generally implicitly validated by, for example, loop conditions, so we usually don't need any further validation, so it's usually more convenient to just return the value.

With limitToDenominator, I generally expect the value to probably be a hard-coded constant (eg Int.MaxValue) or externally-configured value. In the case where the input is untrusted, the validation itself is pretty simple and I don't mind pushing that responsibility to the caller of the method. Given that the list of things that can go wrong is pretty minimal and the validation itself is pretty simple (x > 0), I just feel that it would generally be more annoying for users to deal with an Option here vs just assuming they validated the input in some way already.