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

Use Eval instead of Trampoline for State #782

Merged
merged 2 commits into from
Jan 8, 2016

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented Jan 7, 2016

This may provide the following benefits:

  1. It should be a bit more efficient.
  2. It removes the need to import cats.std.function._ for some
    operations, which may make it a bit more newcomer friendly.
  3. We are already using Eval for things like foldRight, so it seems more
    consistent to use it for State.
  4. I think it's a bit easier to explain Eval than Trampoline, since
    it's hard to explain the latter without first explaining Free. So
    again, this may be a bit more newcomer friendly.

However, there may be downsides I haven't thought of. I thought I would throw this PR together for discussion. Thoughts?

This may provide the following benefits:

1) It should be a bit more efficient.
2) It removes the need to `import cats.std.function._` for some
operations, which may make it a bit more newcomer friendly.
3) We are already using Eval for things like foldRight, so it seems more
consistent to use it for `State`.
4) I think it's a bit easier to explain `Eval` than `Trampoline`, since
it's hard to explain the latter without first explaining `Free`. So
again, this may be a bit more newcomer friendly.
@ceedubs
Copy link
Contributor Author

ceedubs commented Jan 7, 2016

Paging @refried @non @jdegoes @adelbertc @stew @mpilquist as people who have shown interest in State and/or Eval previously.

@codecov-io
Copy link

Current coverage is 88.30%

Merging #782 into master will decrease coverage by -0.18% as of 8d35ddb

@@            master   #782   diff @@
=====================================
  Files          166    166       
  Stmts         2292   2291     -1
  Branches        75     75       
  Methods          0      0       
=====================================
- Hit           2028   2023     -5
  Partial          0      0       
- Missed         264    268     +4

Review entire Coverage Diff as of 8d35ddb

Powered by Codecov. Updated on successful CI builds.

@milessabin
Copy link
Member

👍

I found working with Eval in Kittens a lot more straightforward than using Trampoline.

@ceedubs
Copy link
Contributor Author

ceedubs commented Jan 7, 2016

I noticed there was an unused instance specialized to State. It may have been necessary before this change - I'm not sure - but scalac happily picks up the StateT version, so I've removed it which should fix the reported decrease in code coverage from this PR.

@adelbertc
Copy link
Contributor

👍

adelbertc added a commit that referenced this pull request Jan 8, 2016
Use Eval instead of Trampoline for State
@adelbertc adelbertc merged commit 0529841 into typelevel:master Jan 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants