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

Make Alleycats a submodule of Cats? #472

Closed
non opened this issue Aug 21, 2015 · 22 comments
Closed

Make Alleycats a submodule of Cats? #472

non opened this issue Aug 21, 2015 · 22 comments

Comments

@non
Copy link
Contributor

non commented Aug 21, 2015

The original rationale for keeping Cats and Alleycats in separate repos was to accentuate the differences between the projects' strictness, PR guidelines, etc.

  • Cats' type classes' behavior are underpinned by laws, Alleycats' may not be.
  • Cats' instances are (generally) lawful, Alleycats' may not be.
  • The criteria for including new or novel structures is more relaxed in Alleycats
  • Useful-but-dangerous code is more welcome in Alleycats

However, @inthenow brings up the possibility of moving Alleycats to a submodule of Cats, and there are a few reasons this could be a good idea:

  • The libraries already require lock-step releases, this would make that harder to mess up.
  • Reduce the barrier of moving code into Alleycats from Cats
  • It would help reading code which is split across the two projects (e.g. related to JS futures)
  • Alleycats could take advantage of the documentation machinery/links we are using in Cats

What do you all think? I'm leaning toward keeping the repos separate, but I'm open to being convinced.

@milessabin
Copy link
Member

+1 from me to making alleycats a submodule of cats.

In the work I'm doing on Kittens I'm constantly switching between the two and having them together would make my life easier both from a readability PoV and the lockstep release PoV.

I'd also like to hope that we can come up sufficient inferred laws (via derivation) for at least some alleycats type classes that we could consider moving them into cats.

@dwijnand
Copy link
Contributor

I think it's a good idea.

Both sides of the argument are good, but the 'against' arguments are more project related than repo-related. I'd say with keep the name 'alleycats' to keep accentuating the differences and detail what you've done above in places (README, docs..).

@ghost
Copy link

ghost commented Aug 21, 2015

👍 for alleycats under cats

For the tests for FutureInstances in alleycats I have currently copied the test from Cats. An alternative would be to publish the tests - and that, to me, is going even further in the wrong direction.

@adelbertc
Copy link
Contributor

Unsure myself - I guess my concern is users may bring in alleycats easily seeing it's a proper module of Cats and then freely and happily use the law-breaking instances. I know bringing in cats (without a module suffix) brings in everything, is it possible to exclude alleycats from this and force people who want it to bring it in explicitly?

Also if we do bring it in, perhaps not cats-alleycats.. maybe cats-lawbreaker.. OK maybe not that, but something along those lines :-)

@non
Copy link
Contributor Author

non commented Aug 21, 2015

@adelbertc I think the plan would be to keep the Alleycats code in a separate scala module. So all the imports, etc would remain different. The only difference here is which repo the code lives in, it would still be published under alleycats, imported from alleycats._, etc.

@ghost
Copy link

ghost commented Aug 22, 2015

@adelbertc I understand your concerns.

I was originally going to suggest that alleycats not be part of the cats POM, but rather had to be explicitly specified in the client build.sbt. In further thought though, I think this is overkill, as just because alleycats is in the cats POM , you still have to explicitly import the alleycats modules in scala files (as @non said ^^). So, my personal vote would be "cats" brings in the lot, "alleycats" is not changed to "cats-alleycats" and "build.sbt" authors can choose to bring in the lot or individual libraries.

Also, having used it a little bit, every time I type "alleycat" I do get that "yeah, dodgy" feeling - so the name does it's job for me

@ceedubs
Copy link
Contributor

ceedubs commented Aug 25, 2015

I wouldn't say I'm strongly opposed to this, but it leaves a feeling of being a slight publishing/infrastructure convenience at the cost of bloat. My main concern is this: if we add a binary-incompatible change to alley-cats and want that to be published, does this mean we need to publish a new version of cats? So far we don't have per-module versioning. I know cats is in active development now, but I'm hoping that at some point it can become pretty stable so it's not a nightmare to have as a dependency in a common library. This is especially important as more libraries like circe, monocle, and http4s are considering whether they want cats to be a dependency. Considering this fact, I wonder whether this is really a publishing convenience at all.

@non
Copy link
Contributor Author

non commented Aug 25, 2015

@ceedubs: It's a good question.

So far, the policy is that new releases of Cats require new releases of Alleycats, and that the versions are kept in sync where possible. However, I agree that changes in Alleycats should not force changes in Cats. So I probably overstated the "lock-step releases" part.

We're in a weird state due to being pre-1.0, but in general, I see patch releases in Cats as strictly bug fixes, so in general Cats a.b.x and Alleycats a.b.y should always be compatible, for any x and y.

@ghost
Copy link

ghost commented May 25, 2016

Is this worth adding to the https://github.com/typelevel/cats/milestones/cats-1.0.0 milestone?

@milessabin
Copy link
Member

Bumped by #1059.

@ceedubs
Copy link
Contributor

ceedubs commented May 25, 2016

My main concerns here are those that I stated in #472 (comment) and that I'd be hesitant to have this included in the cats root project. The readme says to add libraryDependencies += "org.typelevel" %% "cats" % "<version>", and I'd hate for people to pick up potentially troublesome instances/types/etc without explicitly opting into them (hopefully after a bit of research into why they are potentially troublesome).

@ghost
Copy link

ghost commented May 25, 2016

@ceedubs But as per #472 (comment) , to actually use them, you have to explicit import them, eg :

import alleycats.Empty._

So the "warning" is quite explicit in the code, far better than in a build file

@ghost
Copy link

ghost commented May 25, 2016

OTOH.....I would rather have them in the cats repo than not in, even if that meant they are not in cats root.

Somewhere, I also think also by @ceedubs, there was also the concern that the root project just pulled in everything, even if people did not want everything - ie cats root should never be used for production, but just as a quick "get started". Given that we should highlight this fact regardless, it would also perhaps be OK to include alleycats as well?

@ceedubs
Copy link
Contributor

ceedubs commented May 25, 2016

@inthenow that's a fair point. Sorry I had forgotten about that and didn't notice it when I was skimming this issue again :)

@benhutchison
Copy link
Member

Coming late to the party.. Im a +1 to this proposal.

I expect that alleycats will become a standard tool in my kit, so having it more integrated with cats releases would suit well. Also Im already observing some close coupling between alleycats and cats code that suggests they should live close by.

@kailuowang
Copy link
Contributor

@non if I understand correctly, in this comment you are suggesting that we can have binary incompatible changes in Alleycats between patch release? I think as long as we communicate that well I am a +1 on this. I've been keeping alleycats in sync with cats in the last couple of releases, it's a bit annoying but not that big a deal.

@edmundnoble
Copy link
Contributor

I am certainly a +1 on this. As long as we exclude alleycats from cats' imports, I don't see an issue here.

@milessabin
Copy link
Member

Also 👍 ... it would be good to have a decision on this before 1.0.

@ghost
Copy link

ghost commented May 10, 2017

Well that's 4 👍 .

Maybe someone should just do a PR on this to move this forward? If nothing appears by the weekend, I'll try and push something for review with the understanding that acceptance is not guaranteed.

@ghost
Copy link

ghost commented May 18, 2017

update: Had to fly back home last weekend, hence did not have the time I thought I would. This coming weekend is free, however.

@kailuowang
Copy link
Contributor

Remove from project 1.0.0-MF as it's consolidated into #1682.

@dwijnand
Copy link
Contributor

Done in #1984.

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

8 participants