-
Notifications
You must be signed in to change notification settings - Fork 158
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
Added NotNaN
predicate
#596
Conversation
Codecov Report
@@ Coverage Diff @@
## master #596 +/- ##
==========================================
+ Coverage 93.08% 93.16% +0.07%
==========================================
Files 65 65
Lines 709 717 +8
Branches 11 10 -1
==========================================
+ Hits 660 668 +8
Misses 49 49
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks @dcastro! I left a few comments, mostly about naming things. :-)
@@ -41,6 +41,9 @@ object numeric extends NumericInference { | |||
/** Predicate that checks if an integral value modulo `N` is `O`. */ | |||
final case class Modulo[N, O](n: N, o: O) | |||
|
|||
/** Predicate that checks if a floating-point number value is not NaN. */ | |||
case class NotNaN() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be final
.
@@ -41,6 +41,9 @@ object numeric extends NumericInference { | |||
/** Predicate that checks if an integral value modulo `N` is `O`. */ | |||
final case class Modulo[N, O](n: N, o: O) | |||
|
|||
/** Predicate that checks if a floating-point number value is not NaN. */ | |||
case class NotNaN() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other negated predicates use Non
as prefix, like NonEmpty
, NonPositive
, NonNegative
, or NonDivisible
. I think we should do it here too and rename it to NonNaN
. It seems to be a common rule to prefix with non- to negate the meaning of a word.
@@ -165,6 +165,16 @@ object numeric { | |||
type NonPosBigDecimal = BigDecimal Refined NonPositive | |||
|
|||
object NonPosBigDecimal extends RefinedTypeOps[NonPosBigDecimal, BigDecimal] | |||
|
|||
/** A `Float` that is not NaN. */ | |||
type FloatNotNaN = Float Refined NotNaN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have NonNegInt
or PosDouble
, this should be NonNaNFloat
.
implicit def floatNotNaNArbitrary[F[_, _]: RefType]( | ||
implicit arb: Arbitrary[Float] | ||
): Arbitrary[F[Float, NotNaN]] = | ||
arbitraryRefType(arb.arbitrary.suchThat(x => !x.isNaN)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick: Using suchThat
could lead in theory to "discarded too many values" errors. I don't think this will ever be a problem in this case but we could eliminate this possibility altogether by mapping NaN
values to 0.0
.
arbitraryRefType(arb.arbitrary.suchThat(x => !x.isNaN)) | |
arbitraryRefType(arb.arbitrary.map(x => if (x.isNaN) 0.0F else x)) |
implicit def doubleNotNaNValidate: Validate.Plain[Double, NotNaN] = fromIsNaN(_.isNaN) | ||
|
||
def fromIsNaN[A](isNaN: A => Boolean): Validate.Plain[A, NotNaN] = | ||
Validate.fromPredicate(x => !isNaN(x), x => s"$x != NaN", NotNaN()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be parenthesis in the string representation of this predicate.
Validate.fromPredicate(x => !isNaN(x), x => s"$x != NaN", NotNaN()) | |
Validate.fromPredicate(x => !isNaN(x), x => s"($x != NaN)", NotNaN()) |
Other predicates do this, too.
Thanks for the feedback @fthomas! I've pushed another commit addressing the comments :) |
As previously discussed in the gitter channel.
There is some duplication for
Float
andDouble
, the only types with a NaN value. I've considered adding aHasNaN[A]
typeclass to reduce duplication, but ultimately thought the complexity was unwarranted in this case. If the maintainers think otherwise, I'd happy to make this change though.