Remove cats from core?

[travisbrown]

@alexarchambault has just submitted a pull request (#28) that removes cats from core and adds a new cats module that includes all of the original type class instances and operations that depended on cats.

This isn't terribly disruptive. Not having a right-biased disjunction is kind of annoying, but in my opinion not annoying enough to justify a dependency on its own. The M versions of withFocus, etc. are nice to have, but aren't used in the core (or in many common use cases). I think I'd be okay with moving all of the cats type class instances to a subproject.

I like the idea of a dependency-free core that just provides the encoding and decoding type classes, an Argonaut-flavored AST, and the zippers, and I'm tempted to go this route (especially now that @alexarchambault has done all the hard work). Is anyone strongly opposed?

[milessabin]

Only possible issue I see is if you were planning to rely more on cats in the very core of circe. That would make this a bit more complicated.

My main concern is that it's quite likely that further developments would make more use of Cats, which would then be forced into the Cats submodule. The net result would be that we would be back to where we are now but with dependencies on two modules rather than one and the additional complexity of attempting to split functionality across an arbitrary boundary.

[travisbrown]

@milessabin Agreed. Here are some of the potential additional uses of cats that I've thought about:

1. Make Decoder a value class wrapper for Kleisli[Xor[DecodingFailure, ?], HCursor, ?] and get a few of its operations from isometric instances.
2. Monoid up JsonObject etc. and get rid of the one-off :+, etc.
3. Give Cursor and history tracking a more explicit Free treatment.
4. If cats someday included a tree structure with keyed branching, use that instead of JsonObject (maybe unrealistic since it would also need to track insertion order, etc.).

So far none of these ideas seem like deal-breakers, but I'd be interested in collecting others.

[travisbrown]

For the record there's additional conversation on this issue in the gitter room.

[ngbinh]

echo @milessabin concern. I am not sure how useful can circe without cats module be?

[travisbrown]

@ngbinh Many (most?) use cases wouldn't require the circe-cats module.

See for example @vkostyukov's new example Finch + circe project here. It uses circe's Shapeless-powered codec derivation and (indirectly through finch-circe) circe-jawn, but it would look literally exactly the same if #28 gets merged.

I think cats is fantastic (maybe that's obvious 😄), and I'm looking forward to adding a cats dependency to Finch, where it will allow us to clean up a lot of code. I just don't see the same kind of value in a cats dependency for circe-core.

[ngbinh]

@travisbrown I see, thanks!

[travisbrown]

My current thinking on this is that we move forward in this direction (cats in a module) as an experiment, but make it clear in the README that cats might be reintroduced into core at some point before 1.0 (or 0.5 or some other earlier cut-off if we think that would be better).

Any objections?

[travisbrown]

One other area where cats would be useful in core is for applicative decoding. I really like the support that Play JSON provides for accumulating errors, and it'd be a nice addition here, but it would be a lot more laborious without a NonEmptyList and syntax for working with applicatives.

[alexarchambault]

@travisbrown I agree that accumulating errors would nice, and that it would be somehow painful to write without the help of cats.

But what could be even nicer would be to be able to choose whether decoding should stop at first error (as it does currently, within the lines of argonaut) or collect all errors that can be. (This could even be done on a type-by-type basis, some stopping at first errors, and others collecting them all.)

In the latter scenario, it feels to me that we would have to get our hands dirty anyway, and that cats would not be of much help here (just a beforehand intuition though...).

[alexarchambault]

The changes I talk about in my previous comment are a bit speculative for now. I don't mind putting the possible split of the core module (into core and cats modules) on hold for now, and the one you're talking about (error accumulation) being given a try.

[travisbrown]

@alexarchambault I could imagine having apply on Decoder return a Validated with accumulated errors, with a new method providing a fast-failing Xor result for cases where you've got something like a large Map[String, Something] in your target type and don't need all the errors.

My conference marathon ended last night, so I'll put together a pull request this morning and we can see if we think it justifies the dependency.

[travisbrown]

We've finally merged error accumulation, only four months after my last comment promising it immediately. 😄

Given that this work depends heavily on cats.data.Validated, I don't think removing cats from core is really an option any more (and it's also not as easy to motivate now that Argonaut is Scalaz-free).