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

Tests for Validated. #329

Merged
merged 2 commits into from
Jun 1, 2015
Merged

Tests for Validated. #329

merged 2 commits into from
Jun 1, 2015

Conversation

woparry
Copy link

@woparry woparry commented May 30, 2015

Ensure failures are combined in order. This required a bugfix.
Also test filter and fromTryCatch since these aren't covered by a typeclass instance and are complex enough to need comments.
Traverse laws are added in the separate PR for #224.

Ensure failures are combined in order. This required a bugfix.
Also test filter and fromTryCatch since these aren't covered by a typeclass instance and are complex enough to need comments.
Traverse laws are added in the separate PR for #224.
@woparry woparry mentioned this pull request May 30, 2015
@non
Copy link
Contributor

non commented May 30, 2015

Looks good to me. 👍


test("ap2 combines failures in order") {
val plus = (_: Int) + (_: Int)
assert(Validated.validatedInstances[String].ap2(Invalid("1"), Invalid("2"))(Valid(plus)) == Invalid("12"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we use Applicative[Validated[String, ?]] instead of Validated.validatedInstances[String]?

The latter one is subject to change if we rename the instance

@julien-truffaut
Copy link
Contributor

minor comments, otherwise it looks very good

@julien-truffaut
Copy link
Contributor

👍

julien-truffaut added a commit that referenced this pull request Jun 1, 2015
@julien-truffaut julien-truffaut merged commit c1f4610 into typelevel:master Jun 1, 2015
@woparry woparry deleted the oparry/288/validated-tests branch June 1, 2015 18:56
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.

4 participants