Instances of NonEmptyReducible are now available for implicit resolution

[akopich]

The type declared for NonEmptyVectorInstances.catsDataInstancesForNonEmptyVector and NonEmptyListInstances.catsDataInstancesForNonEmptyList explicitly declares them to implement NonEmptyReducible trait now. This allows for abstractions over NonEmptyReducible.

[akopich]

Feels like the binary compatibility checker compares only the first type in the chain of mixins, thus reports an error. Or does that actually pose a problem for binary compatibility?
That's indeed a binary compatibility issue.

[codecov-commenter]

Codecov Report

Merging #3541 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3541   +/-   ##
=======================================
  Coverage   91.30%   91.30%           
=======================================
  Files         386      386           
  Lines        8564     8568    +4     
  Branches      246      239    -7     
=======================================
+ Hits         7819     7823    +4     
  Misses        745      745           

[barambani]

👍 on green and no conflicts. I'm linking this to milestone 3 as the original implementation will be preferable when bincompat will not be an issue any more.

[akopich]

@barambani so shall we just wait until cats 3?
Should I revert all the commits but the original one?

[barambani]

No this version adds value so I would merge it in in this compatible way. I think the only conflicts are due to the port to munit in the tests (but I haven't pulled you branch). I linked this PR to milestone 3 so that will be easier to remember to implement it in the original way when we will be free from the bincompat constraint. Sorry I didn't explain myself.

[akopich]

@barambani thanks for the explanation.

Should I resolve the merge conflict?

[barambani]

we could move this below NonEmptyVectorInstances like it's done in NonEmptyListInstances0. It's pretty conventional across the board and makes the read top to bottom easier.

[barambani]

now that we have this we could write split in here in terms of it. Something like

override def split[A](fa: NonEmptyList[A]): (A, List[A]) = catsDataNonEmptyReducibleNonEmptyList.split(fa)

and we could do the same for NonEmptyVector. The reason I'm suggesting this is I wouldn't want for someone to change the implementation of split in one place and get some inconsistent behaviour on the other instances.

[akopich]

Thanks for the great point. Code duplication is never good.

[barambani]

Yes please, I also added two very minor comments. Thanks.

[akopich]

@barambani I've addressed your comments and resolved the merge conflicts.

[barambani]

👍

[travisbrown]

This has come up a few times before (#3069, #3404) and the decision has been that NonEmptyReducible is a convenience trait for implementers, not a type class. I don't think we're stuck with that decision forever, necessarily, but I think there needs to be a clear reason to change it.

[akopich]

@travisbrown
My motivation came from a need to average objects --that is, reduce the collection and divide by its size. Reducible typeclass would make it possible to abstract over non-empty containers, if it implemented size method.

[barambani]

@akopich apologies, I wasn't aware of these previous attempts and the final decision.

[akopich]

@barambani nevermind.

Anyway, doesn't the fact that the discussion of the matter has been initiated at least three times in the last year justify a need to expose at least some functionality of NonEmptyReducible? I'd like to expose size method at least. Is there a typeclass like HasSize?

[joroKr21]

But Reducible has a size method.

[travisbrown]

The only method NonEmptyReducible adds to Reducible is split, and while I think we still don't have a great story for abstracting over non-empty things, I also don't think split / NonEmptyReducible is the right answer. For one thing the G[_] part should be a type parameter if that was the goal, so that it could act as a functional dependency.

Personally I'd prefer to see us make NonEmptyReducible package-private or at least add documentation to clarify that it's only a convenience trait for Reducible definitions, not a type class.

[akopich]

@joroKr21 yeah, my bad. Actually I needed the head-tail decomposition, so I could implement size(nonempty): PositiveInt in constant time. Thanks for pointing that out.

[akopich]

@travisbrown but wouldn't making NonEmptyReducible package-private break user's code? Or do you mean, this should be done in cats 3?

[travisbrown]

@akopich Right, it'd definitely break source-compatibility, but not binary compatibility, so in theory it could be done before Cats 3 (I doubt it's widely used outside of Cats, and could be provided in a compat library).

I don't think it's really necessary to go that far, though—I just agree with you that this is confusing, and my preference would be to fix it as clearly as possible.

[akopich]

@travisbrown IMO we can drastically decrease the number of similar PRs with a "it's not a bug" comment above the lines instantiating catsDataInstancesForNonEmptyList and catsDataInstancesForNonEmptyVector.

[travisbrown]

@akopich Would you want to open a PR adding that, and maybe a comment for NonEmptyReducible itself? I've been meaning to do that since last September, but clearly am not doing a very good job of it. 😄

[akopich]

@travisbrown ... and also add @deprecated? Or should the annotation rather be added only when it's already decided in which version the entity will be removed/made private?

[travisbrown]

I think the documentation can happen first. We do want to use it internally, so I'm not sure deprecation is the right approach.

[akopich]

@travisbrown #3562