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

Restructure arrow and functor package. #1930

Closed
kailuowang opened this issue Sep 25, 2017 · 16 comments
Closed

Restructure arrow and functor package. #1930

kailuowang opened this issue Sep 25, 2017 · 16 comments
Assignees

Comments

@kailuowang
Copy link
Contributor

kailuowang commented Sep 25, 2017

  1. move arrow to a separate module.
  2. Move Profunctor and Strong to this module arrow
  3. Move Bifunctor, Invariant and Contravariant to core
@kailuowang
Copy link
Contributor Author

Please vote if you, like me, think it's mostly bikeshedding. But if you have stronger feeling to put down, please do, it will be helpful for sure.

@tpolecat
Copy link
Member

If you agree with my organizing principle for the infographic the outliers are pretty evident I think.

@kailuowang
Copy link
Contributor Author

looks like the decision can be made. I am moving this issue to "ready"

@kailuowang kailuowang self-assigned this Sep 26, 2017
@kailuowang
Copy link
Contributor Author

working on a PR.

@kailuowang
Copy link
Contributor Author

arrow.FunctionK depends on cats.core, I incline to keep it in cats.core but it means that I need to move it to the cats root package. cc @sellout. Speaking of FunctionK its documentation is in the data types section, but I am not sure if that's accurate.

@sellout
Copy link
Contributor

sellout commented Sep 26, 2017

@kailuowang Having FunctionK in core sounds good to me.

@LukaJCB
Copy link
Member

LukaJCB commented Sep 26, 2017

I think adding it to data does make some sense, as we have mostly type classes in cats . Don't have any strong opinion either way though.

@kailuowang
Copy link
Contributor Author

the part I am not sure about is if FunctionK is a data type. Anyway moving arrow to a module is more complicated than I thought, since Category and Compose depends on SemigroupK and MonoidK, so more triage needed for that one. I'll submit one PR for restructuring the functor package first.

@LukaJCB
Copy link
Member

LukaJCB commented Sep 26, 2017

Well, I'd say it's as much a data type as Kleisli and cokleisli, no? I think we could put everything that's not a type class into data :D

@sellout
Copy link
Contributor

sellout commented Sep 26, 2017

@kailuowang

  1. I think having FunctionK in core/data is the right place
  2. It should be fine for the arrow module to depend on the core module, no?

But yes, get rid of functor first 👍🏾

@kailuowang
Copy link
Contributor Author

If I am not mistaken, Core depends on arrow, so circular intermodule dependency? I am okay with functionK going too data

@sellout
Copy link
Contributor

sellout commented Sep 26, 2017

@kailuowang Ah, yeah – it looks like the syntax stuff depends on arrow. That should be pulled out too if arrow is a separate module. But then I guess we’d have cats.arrow.implicits?

It’d be good to rethink the relegation of implicit conversions to their own packages. SlamData did at least a couple experiments where we eliminated the wildcard imports, and it had no noticeable effect on compilation time.

@kailuowang
Copy link
Contributor Author

Kleisli, Cokleisli and As have instances of type classes in arrow. They also use it to get the MonoidK and SemigroupK instance from their algebraK. (hence the cycle)
If we actually do get rid off MonoidK and SemigroupK with newtype, then this problem might be solved, since algebraK is the only dependency from arrow to core.

@sellout
Copy link
Contributor

sellout commented Sep 27, 2017

@kailuowang Thanks for the clear explanation. Yeah, I almost mentioned the newtype thing (which am also in favor of).

@kailuowang
Copy link
Contributor Author

@sellout , just realized that you are okay with the arrow module depending on the core module, what is your incentive to move it out of core again?

@LukaJCB
Copy link
Member

LukaJCB commented Aug 5, 2018

I think this can be closed now, right? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants