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

Move cats-free and cats-state in to cats-core #765

Merged
merged 3 commits into from
Jan 4, 2016

Conversation

DavidGregory084
Copy link
Member

To address #760

@codecov-io
Copy link

Current coverage is 88.64%

Merging #765 into master will increase coverage by +0.13% as of a45cb38

@@            master    #765   diff @@
======================================
  Files          166     166       
  Stmts         2289    2317    +28
  Branches        74      74       
  Methods          0       0       
======================================
+ Hit           2026    2054    +28
  Partial          0       0       
  Missed         263     263       

Review entire Coverage Diff as of a45cb38

Powered by Codecov. Updated on successful CI builds.

@mpilquist
Copy link
Member

👍

1 similar comment
@milessabin
Copy link
Member

👍

@mikejcurry
Copy link
Contributor

Minor, but I had added the following text to the contributors guide a short time ago: "Tests for additional modules, such as free, go into the tests directory within that module."

If this goes through, and free moves into core, then it might make sense to change the above text to refer to the jvm module instead of referring to free, as free will no longer be a separate module and the tests have been factored to the main tests directory. I can push through a separate change at some stage if it doesn't make this one, but for consistency it might be better to change the docs and the code together.

@DavidGregory084
Copy link
Member Author

Thanks for pointing that out; not near a PC at the minute but I'll try and address the documentation changes this evening.

@non
Copy link
Contributor

non commented Jan 4, 2016

Rerunning tests now. 👍 once they pass.

non added a commit that referenced this pull request Jan 4, 2016
Move cats-free and cats-state in to cats-core
@non non merged commit 9b48dbb into typelevel:master Jan 4, 2016
@non
Copy link
Contributor

non commented Jan 4, 2016

Thanks!

@DavidGregory084 DavidGregory084 deleted the free-state-to-core branch January 6, 2016 20:17
ceedubs added a commit to ceedubs/cats that referenced this pull request Apr 23, 2016
It's quite possible that people are going to throw tomatoes at me for
proposing this change. There has been much discussion about
modularization in the past, and in fact free used to have its own
module, which was removed in typelevel#760/typelevel#765.

One significant change that has happened since then is that `State` has
started to use `Eval` instead of `Trampoline` for trampolining. At this
point, nothing outside of the `free` package in `cats-core` depends on
`free` (which is why this PR was so easy to create). To me that makes it
a fairly convenient border for modularization.

There was a [brief
discussion](https://gitter.im/typelevel/cats?at=5716322b548df1be102e3f3c) about this on Gitter in which @sellout supported this change on the basis that they prefer to use fixpoint types (such as `Mu`) for free structures. Putting `free` into a separate module may be better for people who want to use different variations of these structures.

Of course in addition to these more recent updates, there are the usual
arguments for modularization and decreased jar size (which I believe is
especially important for `catsJS` users).
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.

6 participants