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

Example for Validated.ap and swap combine order #2074

Closed
wants to merge 1 commit into from

Conversation

markus1189
Copy link
Contributor

Okay this was strange, why was the order of arguments in the combine swapped?

24 Pull Requests (https://24pullrequests.com)

https://24pullrequests.com

@ceedubs
Copy link
Contributor

ceedubs commented Dec 6, 2017

@markus1189 ah thanks for pointing this out. This is confusing. Strangely enough, this ap implementation is "right", but what's strange about it is that it's kind of in the reverse order as the ap method defined in Apply. See #803 and the chain of related issues/PRs. I would imagine that we have some similar headscratchers for other data types.

One solution to this might be to change the signature to require that the Validated instance that you are calling this on is of the form Validated[E, A => B]. A possibly more straightforward solution would be to just remove this method from Validated and let people access it via type class syntax if they want it. I can't think of a single time that I've ever seen anyone use ap directly in application code. I think that it's far more common to use map2, product, etc.

@ceedubs
Copy link
Contributor

ceedubs commented Dec 7, 2017

Related failures in the build:

[info] *** 4 TESTS FAILED ***

[info] ParallelSuite:

[info] 

[info] - ApplicativeError[Validated[String, Int]].applicativeError.ap consistent with product + map *** FAILED ***

[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.

[info]    (Discipline.scala:14)

[info]     Falsified after 6 successful property evaluations.

[info]     Location: (Discipline.scala:14)

[info]     Occurred when passed generated values (

[info]       arg0 = Invalid(),

[info]       arg1 = Invalid(᧞줧)

[info]     )

[info]     Label of failing property:

[info]       Expected: Invalid(᧞줧)

[info]   Received: Invalid(᧞줧)

[info] ParallelSuite:

[info] 

[info] - parAp accumulates errors in order *** FAILED ***

[info]   Left("WorldHello") did not equal Left("HelloWorld") (ParallelSuite.scala:98)

[info] NestedSuite:

[info] 

[info] - Nested[Validated[String, ?], ListWrapper, ?].applicativeError.ap consistent with product + map *** FAILED ***

[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.

[info]    (Discipline.scala:14)

[info]     Falsified after 11 successful property evaluations.

[info]     Location: (Discipline.scala:14)

[info]     Occurred when passed generated values (

[info]       arg0 = Nested(Invalid(죉玔䒛)),

[info]       arg1 = Nested(Invalid(夒ㄳㇶ))

[info]     )

[info]     Label of failing property:

[info]       Expected: Nested(Invalid(夒ㄳㇶ죉玔䒛))

[info]   Received: Nested(Invalid(죉玔䒛夒ㄳㇶ))

[info] ValidatedSuite:

[info] 

[info] - Validated[String, Int].applicativeError.ap consistent with product + map *** FAILED ***

[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.

[info]    (Discipline.scala:14)

[info]     Falsified after 27 successful property evaluations.

[info]     Location: (Discipline.scala:14)

[info]     Occurred when passed generated values (

[info]       arg0 = Invalid(㱈ѥ躨㟫볓),

[info]       arg1 = Invalid(뼊᪯)

[info]     )

[info]     Label of failing property:

[info]       Expected: Invalid(뼊᪯㱈ѥ躨㟫볓)

[info]   Received: Invalid(㱈ѥ躨㟫볓뼊᪯)

@markus1189
Copy link
Contributor Author

@ceedubs Yes this was confusing :D But thinking of it in Haskell terms I can see how the confusion arises.

I would vote for removing the ap from Validated, it seems like there is no added convenience.

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.

2 participants