-
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
Add .validate method for refined companions #382
Conversation
Codecov Report
@@ Coverage Diff @@
## master #382 +/- ##
=========================================
+ Coverage 96.58% 96.6% +0.01%
=========================================
Files 41 42 +1
Lines 498 500 +2
Branches 11 10 -1
=========================================
+ Hits 481 483 +2
Misses 17 17
Continue to review full report at Codecov.
|
Looks great, thanks @howyp! This would also resolve #75 although that one is about adding a method to |
@@ -11,16 +12,26 @@ class CatsSpec extends Properties("cats") { | |||
val refTypeOrder: Unit = () // shadow the `Order` instance so the `Eq` instance is tested | |||
locally(refTypeOrder) // prevent a "unused" warning | |||
|
|||
PosInt.unsafeFrom(5) === PosInt.unsafeFrom(5) |
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.
I'm using unsafeFrom
in the tests to avoid the overhead of the RefineMacro
so that test code compiles faster.
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.
Ah, ok, I'll revert
object validation { | ||
implicit class CatsValidateOps[FTP, T](companion: RefinedTypeOps[FTP, T]) { | ||
def validate(value: T): ValidatedNel[String, FTP] = | ||
Validated.fromEither(companion.from(value)).leftMap(NonEmptyList.one) |
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 is a toValidatedNel
on Cats' EitherSyntax
which would allow us to just write companion.from(value).toValidatedNel
here.
I'm totally fine with using |
Not sure about packaging here - do we need a |
I'm not sure if I like d59f2d8. It adds a Regarding |
How about some scalaz love? ;) I can take it up if you want, @howyp. |
Adding this functionality to the Scalaz module would be nice too. I would prefer a separate PR for that. |
@fthomas Alright, I'll graft the code from this one into the scalaz module. |
I can understand why you might feel hesitant about having an operator on anything (though it would only work on types which have a matching refined predicate). It seemed to me to be quite a 'cats' way of doing things - in the style of say import validation._
val myInt = 5
validate[Positive](myInt) Would you prefer that for the moment? My motivation was to provide symmetrical support for cases where you haven't defined a full refined type/don't want to create companion objects, but just want to be able to apply a predicate to a raw type. I agree it would be good to distill the API surface of Refined, and that standardising on companion objects for refined types is a great way to go. But only providing second-class support for ad-hoc refinements via a predicate seems a shame to me. Perhaps how to reduce the surface of the API would be best discussed in a new issue? |
@howyp I think |
d59f2d8
to
bb93a9d
Compare
Ok, I've dropped the |
Ok, merging now. Thanks again @howyp! |
Now we have 'companion' objects for refined types, I thought it would be nice to have a neat way to refine to cat's
Validated
type.It always uses
NonEmptyList
on the error side at the moment. I guessed that this is what people will use 90% time but if we think there will also be need for usingList
or evenList Refined Not[Empty]
then I we could support that too?