Weaken some Alternative constraints in OneAnd

[kory33]

Currently, the unwrap and combine methods in OneAnd, as well as SemigroupK / Semigroup instance have Alternative as a bound for the type F[_] of collection-like component.

This pull request weakens this constraint to F[_]: NonEmptyAlternative.

Context

The change allows one to unwrap OneOf[NonEmptyList, A] into NonEmptyList[A]. At first glance putting NonEmptyList in OneOf sounds a bit weird, but I think they are natural choices for situations when one wants to describe a collection of 2..N items (this occurred to me while describing "duplicate inputs error" in a validation logic, where the error case class had a length ≥2 list of input indices containing duplicates).

I think this weakening generalization makes sense since, intuitively, none of the aforementioned methods interact with the empty: F[Nothing] aspect of the collection: they only combine collections (SemigroupK) or inject an element into the collection (NonEmptyAlternative).

[kory33]

[question] Is this the right way to pass the MiMa check for a case class modification?

I saw #3997 (comment) and thought that this was the (only) way, but I could not find any other place that defines a similar bincompat trait (other than ValidatedFunctionsBinCompat0, which patches the companion rather than the Validated class itself).

[johnynek]

can we make this sealed? I think so.

[kory33]

Thanks, I made it sealed : af23366

[kory33]

[question] Is since = "2.14.0" specification appropriate (is that the next cats version)? Or do I just not need @deprecated at all since it's private[data]?

[johnynek]

personally I wouldn't add the deprecated and just add a comment.

[satorg]

If a method is superceded by some other method with the same name, we usually not only make it package local, but also remove all implicit keywords from its definition, e.g.:

private[data] def combine(other: OneAnd[F, A])(F: Alternative[F]): OneAnd[F, A] = ...

which makes it less likely to interfere with the new method while preserving binary compatibility.

I personally don't mind marking it @deprecated but don't have strong opinion on that – it is not supposed to be called anyway.

[kory33]

Oh, I thought I made the bincompat check pass but I didn't! I'll try to fix it.

[johnynek]

personally I wouldn't add the deprecated and just add a comment.

[johnynek]

can we make this sealed? I think so.

[satorg]

Abstract vals look suspicious a bit. And might not be necessary – since OneAndBinCompat0 is supposed to be extended by OneAnd only, a trick with self-type annotation might work here:

private[data] trait OneAndBinCompat0[F[_], A] {
  self: OneAnd[F, A] => // allow access to `head` and `tail` from withing this trait
    // no abstract `head` and `tail` should be necessary

[kory33]

For some reason I've never used a non-trait type for self-type annotation and couldn't come up with this!
Applied: af23366

[satorg]

While not a requirement, but since there's self: OneAnd in the scope and because Alternative extends NonEmptyAlternative, I wonder if it could be simplified even further:

Suggested change

	private[data] def unwrap(implicit F: Alternative[F]): F[A] =
	F.prependK(head, tail)
	// Kept for binary compatibility
	private[data] def combine(other: OneAnd[F, A])(implicit F: Alternative[F]): OneAnd[F, A] =
	OneAnd(head, F.combineK(tail, other.unwrap))
	private[data] def unwrap(implicit F: Alternative[F]): F[A] =
	self.unwrap(F)
	// Kept for binary compatibility
	private[data] def combine(other: OneAnd[F, A])(F: Alternative[F]): OneAnd[F, A] =
	self.combine(other)(F)

Also, I would still recommend to go without implicit parameters here because even though these methods are private[data] they are still visible to Cats' internals and may cause unexpected unexpecties in the future. Not a requirement either, though.

[kory33]

That makes sense. I think the same applies to OneAndInstancesBinCompat0 as well, so I made a similar change to the instances trait too (e059ed8).

[satorg]

Thank you!