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

Remove Applicative#pureEval #1234

Merged
merged 1 commit into from
Jul 27, 2016
Merged

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented Jul 27, 2016

This probably resolves #1150, though it doesn't provide an alternative
type class as was brought up there.

@adelbertc has gotten tripped up by pureEval defaulting to a call to pure with .value called on the Eval.

@johnynek has also questioned pureEval being directly on Applicative.

@tpolecat has raised concerns that pureEval looks like it might exist as a way to inject
side effects into a type, but that it doesn't actually make any claims
about execution semantics or value retention, so it can't be relied upon
to do this.

I think that there could be an argument for having an instantiation method for
a side-effect-y type (like a Task) that takes an Eval[A] and produces an instance
that will repeat the computation given Always, memoize the value given
Later, etc. However, I don't think that @tpolecat necessarily agrees
with me, and I think that even if this is a reasonable thing to do,
Applicative probably isn't the place for it.

Since several people have questioned pureEval, it was entirely
untested, and I haven't heard any one argue for it staying, I'm inclined
to remove it. If someone has a compelling example of when a non-strict
argument can be useful for pure, then I may be convinced. But even
then we should clearly document what behavior can be expected of
pureEval.

@non I believe that you originally added pureEval as part of your StreamingT work. Do you have any thoughts on this?

This probably resolves typelevel#1150, though it doesn't provide an alternative
type class as was brought up there.

@adelbertc has [gotten tripped up](typelevel#1150 (comment)) by `pureEval` defaulting to a call to `pure` with `.value` called on the `Eval`.

@johnynek has also [questioned](typelevel#1150 (comment)) `pureEval` being directly on `Applicative`.

@tpolecat has [raised concerns](https://gitter.im/typelevel/cats?at=5794dc4c1b9de56c0edd887a) that `pureEval` looks like it might exist as a way to inject
side effects into a type, but that it doesn't actually make any claims
about execution semantics or value retention, so it can't be relied upon
to do this.

I think that there could be an argument for having an instantiation method for
a side-effect-y type (like a `Task`) that takes an `Eval[A]` and produces an instance
that will repeat the computation given `Always`, memoize the value given
`Later`, etc. However, I don't think that @tpolecat necessarily agrees
with me, and I think that even _if_ this is a reasonable thing to do,
`Applicative` probably isn't the place for it.

Since several people have questioned `pureEval`, it was entirely
untested, and I haven't heard any one argue for it staying, I'm inclined
to remove it. If someone has a compelling example of when a non-strict
argument can be useful for `pure`, then I may be convinced. But even
then we should clearly document what behavior can be expected of
`pureEval`.
@codecov-io
Copy link

Current coverage is 89.77% (diff: 100%)

Merging #1234 into master will increase coverage by 0.14%

@@             master      #1234   diff @@
==========================================
  Files           234        234          
  Lines          3143       3139     -4   
  Methods        3089       3082     -7   
  Messages          0          0          
  Branches         51         54     +3   
==========================================
+ Hits           2817       2818     +1   
+ Misses          326        321     -5   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update a2d8f60...94fcf03

@johnynek
Copy link
Contributor

👍

3 similar comments
@tpolecat
Copy link
Member

👍

@adelbertc
Copy link
Contributor

👍

@kailuowang
Copy link
Contributor

👍

@kailuowang kailuowang merged commit 910a42a into typelevel:master Jul 27, 2016
@kailuowang
Copy link
Contributor

I apologize @ceedubs and @non, I forgot that @non is mentioned here as well, my bad. I will do the fix if needed.

@kailuowang kailuowang self-assigned this Jul 27, 2016
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.

pureEval vs a typeclass
6 participants