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

Separate free package into its own module #990

Merged
merged 5 commits into from
May 5, 2016

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented 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 #760/#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 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).

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).
@ceedubs
Copy link
Contributor Author

ceedubs commented Apr 23, 2016

I'm particularly interested in feedback from @mpilquist, @travisbrown, @stew, @refried, and @milessabin, as they had previously approved of pulling free into core (but a bit has changed since then as mentioned in the description above).

@milessabin
Copy link
Member

I think that Eval taking over from Trampoline makes a world of difference. This gets a 👍 from me.

@ceedubs
Copy link
Contributor Author

ceedubs commented Apr 23, 2016

Another item that I meant to mention is #845. Basically there are many more free structures that could potentially be added, and if we choose to go down that path we might not want to add too much "bloat" to core.

@codecov-io
Copy link

codecov-io commented Apr 23, 2016

Current coverage is 81.99%

Merging #990 into master will not change coverage

@@             master       #990   diff @@
==========================================
  Files           215        215          
  Lines          2704       2704          
  Methods        2639       2639          
  Messages          0          0          
  Branches         60         60          
==========================================
  Hits           2217       2217          
  Misses          487        487          
  Partials          0          0          

Powered by Codecov. Last updated by 8355a19...c11ddcf

@julien-truffaut
Copy link
Contributor

What about moving free to it's own project? it seems there is quite a lot of development in this area and it might be easier to experiment without being locked with cats releases.

@adelbertc
Copy link
Contributor

👍 from me

@adelbertc
Copy link
Contributor

Woah there -2.61%

@mpilquist
Copy link
Member

👍, either for a separate module or even a separate repo.

@ceedubs
Copy link
Contributor Author

ceedubs commented May 5, 2016

@adelbertc validateJVM wasn't running the free tests. I've fixed it, and codecov now shows no difference in coverage (though it didn't post an updated comment).

I think I'm going to add some notes in the relevant documentation about this being in a separate module and then merge this. I'll post a follow-up issue about whether it should be in its own project, but I don't want that indecision to keep it in core when it sounds like people are agreed that it shouldn't be there.

@ceedubs ceedubs merged commit 9a96070 into typelevel:master May 5, 2016
@ceedubs ceedubs deleted the a-hobbits-tale branch May 5, 2016 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants