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

Validation #22

Merged
merged 7 commits into from Jul 22, 2016

Conversation

Projects
None yet
3 participants
@boris-marinov
Contributor

boris-marinov commented Jun 28, 2016

Done except the functions for converting it to Either and (I think also good to have) Maybe.

Suggestion: instead of having fromEither function, isn't it better if we have toValidation method on the Either datatype (and vise-versa). This way you woudn't have to include a module in order to do the conversion, and also seems to me more correct in therms of design.

Looking forward for input on that.
Also what did you have in mind for the Conversions module in #5?

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jun 28, 2016

Contributor

Oh, I have to do the concat function as well.

Contributor

boris-marinov commented Jun 28, 2016

Oh, I have to do the concat function as well.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jul 6, 2016

Member

Ah, so the Conversions module was there to avoid this problem of "do we have an Either.fromValidation, or do we have a Validation.toEither?", it also avoids problems with circular dependencies.

Otherwise we'd ideally have both Either.fromValidation and Validation.toEither.

Member

robotlolita commented Jul 6, 2016

Ah, so the Conversions module was there to avoid this problem of "do we have an Either.fromValidation, or do we have a Validation.toEither?", it also avoids problems with circular dependencies.

Otherwise we'd ideally have both Either.fromValidation and Validation.toEither.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jul 6, 2016

Contributor

OK, here is a concat function (not sure how to handle concating several Success vals but I assume that the emphasis here is on accumulating errors).

I will work on the conversions in a separate branch.

Contributor

boris-marinov commented Jul 6, 2016

OK, here is a concat function (not sure how to handle concating several Success vals but I assume that the emphasis here is on accumulating errors).

I will work on the conversions in a separate branch.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jul 6, 2016

Member

Looks reasonable. I think we could also add .empty() as Failure([]) and make it a monoid over errors.

Member

robotlolita commented Jul 6, 2016

Looks reasonable. I think we could also add .empty() as Failure([]) and make it a monoid over errors.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jul 10, 2016

Contributor

The funny thing is that an empty Success instance can serve as a mempty for a Validation monoid definition:

Failure(a).concat(Success()).equals(Failure(a))

Just an idea.

Contributor

boris-marinov commented Jul 10, 2016

The funny thing is that an empty Success instance can serve as a mempty for a Validation monoid definition:

Failure(a).concat(Success()).equals(Failure(a))

Just an idea.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jul 11, 2016

Member

Ah, that's interesting. We could make it Success(x => x), so it's also an identity for the Applicative instance. Unless there's some law it breaks that I'm not aware of.

Member

robotlolita commented Jul 11, 2016

Ah, that's interesting. We could make it Success(x => x), so it's also an identity for the Applicative instance. Unless there's some law it breaks that I'm not aware of.

@vendethiel

This comment has been minimized.

Show comment
Hide comment
@vendethiel

vendethiel Jul 11, 2016

I think that's how it makes sense. If you have mempty be a Failure([]), it means Success().concat(Validation.mempty()) would be Failure, right?

vendethiel commented Jul 11, 2016

I think that's how it makes sense. If you have mempty be a Failure([]), it means Success().concat(Validation.mempty()) would be Failure, right?

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jul 11, 2016

Contributor

@vendethiel , I thing robotlolita was suggesting that Failure([]) can serve as mempty, just for Failure not for Validation.

@robotlolita, aren't the applicative laws concerned with just ap and of?

Regarding the monoid laws note that if Validation.empty = () => Success(Something) as I suggested then they get broken:

Success(a).concat(Validation.empty()).equals(Success(a)) // false

Actually we can fix this by extending the concat function of Success, so instead of

Success.concat = function(a) {
  return a
}

make it into

Success.concat = function(a) {
  a.equals(Validation.empty()) ? this : a
}

Conceptually, I think all Success values are mempty, but the interface does not work that way. :)

Contributor

boris-marinov commented Jul 11, 2016

@vendethiel , I thing robotlolita was suggesting that Failure([]) can serve as mempty, just for Failure not for Validation.

@robotlolita, aren't the applicative laws concerned with just ap and of?

Regarding the monoid laws note that if Validation.empty = () => Success(Something) as I suggested then they get broken:

Success(a).concat(Validation.empty()).equals(Success(a)) // false

Actually we can fix this by extending the concat function of Success, so instead of

Success.concat = function(a) {
  return a
}

make it into

Success.concat = function(a) {
  a.equals(Validation.empty()) ? this : a
}

Conceptually, I think all Success values are mempty, but the interface does not work that way. :)

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jul 18, 2016

Contributor

Fixed the issues that you found.
Fixed the isssue with the monoid laws breaking.

Contributor

boris-marinov commented Jul 18, 2016

Fixed the issues that you found.
Fixed the isssue with the monoid laws breaking.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jul 18, 2016

Member

Hm, this is a confusing Monoid instance to me:

Success(1).concat(Validation.empty()) = Success(1)
Success(1).concat(Success(x => x)) ≃ Success(x => x)
Success(x => x) ≃ Validation.empty()

Granted function equivalence is a tricky thing. Maybe leave this out of the default Validation for now and revisit it later? I've got some ideas for multiple instances that I still have to write about.

Member

robotlolita commented Jul 18, 2016

Hm, this is a confusing Monoid instance to me:

Success(1).concat(Validation.empty()) = Success(1)
Success(1).concat(Success(x => x)) ≃ Success(x => x)
Success(x => x) ≃ Validation.empty()

Granted function equivalence is a tricky thing. Maybe leave this out of the default Validation for now and revisit it later? I've got some ideas for multiple instances that I still have to write about.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jul 19, 2016

Contributor

The aforementioned problem with conforming to monoid laws, is already fixed.

Do you still think that we should leave out the empty.

Contributor

boris-marinov commented Jul 19, 2016

The aforementioned problem with conforming to monoid laws, is already fixed.

Do you still think that we should leave out the empty.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jul 20, 2016

Member

Ah, sorry, I could've been more clear. What I meant is that the monoid instance is valid, but it's confusing to me because it relies on identity rather than structural equality, so I'd prefer to leave it out of the default.

I'd be okay with providing a separate instance that people can import/opt-in for explicitly, but we don't have a good way of doing that yet.

Member

robotlolita commented Jul 20, 2016

Ah, sorry, I could've been more clear. What I meant is that the monoid instance is valid, but it's confusing to me because it relies on identity rather than structural equality, so I'd prefer to leave it out of the default.

I'd be okay with providing a separate instance that people can import/opt-in for explicitly, but we don't have a good way of doing that yet.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov Jul 20, 2016

Contributor

I agree.

Contributor

boris-marinov commented Jul 20, 2016

I agree.

@robotlolita robotlolita merged commit 27b6439 into master Jul 22, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita Jul 22, 2016

Member

Thanks :)

Member

robotlolita commented Jul 22, 2016

Thanks :)

@robotlolita robotlolita deleted the validation branch Jul 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment