Optimize Alternative (part 4): add prependK/appendK specializations for Cats monad transformers

[satorg]

Adds optimized specializations for the new prependK/appendK methods introduced in the initial PR (#4014).

Previous PRs:

• Optimize Alternative (part 2): add prependK/appendK specializations for std containers #4052.
• Optimize Alternative (part 3): add prependK/appendK specializations for Cats NE wrappers #4055

[satorg]

The PR is created as "Draft" because this work was done quite a long ago and I just want to make sure I didn't lose something after lots of merges and rebases. But perhaps it is good to go already.

[satorg]

I've made a deep inhale and re-reviewed the changes here. I would say they look good to me and seems all the tests and Mima checks are happy as well. So I would dare to claim the PR is ready to go)

[satorg]

I think that all the concerns have been addressed here. Could you take one more look please?

[armanbilge]

Sorry, haven't forgotten. Rather than "one more look" it needs "one proper look" actually 😅

[armanbilge]

Sorry if I missed it. Did we add law tests for this instance?

[satorg]

Yep, no worries, here they are:

cats/laws/src/main/scala/cats/laws/NonEmptyAlternativeLaws.scala

Line 27 in f2d871c

trait NonEmptyAlternativeLaws[F[_]] extends ApplicativeLaws[F] with SemigroupKLaws[F] {

cats/laws/src/main/scala/cats/laws/discipline/NonEmptyAlternativeTests.scala

Line 30 in f2d871c

trait NonEmptyAlternativeTests[F[_]] extends ApplicativeTests[F] with SemigroupKTests[F] {

[armanbilge]

Thanks, I mean specifically for the IndexedReaderWriterStateT instance of NonEmptyAlternative.

[satorg]

Accordingly, AlternativeLaws and AlternativeTests were changed to extend NonEmptyAlternative*:

cats/laws/src/main/scala/cats/laws/AlternativeLaws.scala

Line 28 in f2d871c

trait AlternativeLaws[F[_]] extends NonEmptyAlternativeLaws[F] with MonoidKLaws[F] {

cats/laws/src/main/scala/cats/laws/discipline/AlternativeTests.scala

Line 30 in f2d871c

trait AlternativeTests[F[_]] extends NonEmptyAlternativeTests[F] with MonoidKTests[F] {

[satorg]

@armanbilge is it what you're asking about?

cats/tests/shared/src/test/scala/cats/tests/IndexedReaderWriterStateTSuite.scala

Lines 451 to 466 in f2d871c

	{
	implicit val G: Monad[ListWrapper] = ListWrapper.monad
	val SA = IRWST.catsDataAlternativeForIRWST[ListWrapper, Boolean, String, MiniInt](ListWrapper.monad,
	ListWrapper.alternative,
	Monoid[String]
	)
	checkAll(
	"IndexedReaderWriterStateT[ListWrapper, String, String, Int, Int, *]",
	AlternativeTests[IRWST[ListWrapper, Boolean, String, MiniInt, MiniInt, *]](SA).alternative[Int, Int, Int]
	)
	checkAll("Alternative[IndexedReaderWriterStateT[ListWrapper, String, String, Int, Int, *]]",
	SerializableTests.serializable(SA)
	)
	}

[satorg]

That part of tests was not changed, though, iirc – it included tests for NonEmptyAlternativeLaws automatically due to the inheritance.

[armanbilge]

I see, thanks. I was asking, because for the other instances you added tests using ListWrapper.nonEmptyAlternative in addition to existing tests using the ListWrapper.alternative, as far as I can tell.

[satorg]

Oh, I see... That's a fair concern.. And honestly, I don't remember why. There are three causes possible:

1. I simply forgot to add it.
2. I lost it after a series of successive rebases.
3. Most likely, little green men came and stole that piece of code from the repo.

But I think, despite of the cause, it makes sense to return it back.

[satorg]

@armanbilge I added direct NonEmptyAlternativeLaws tests for IRWST type.
But besides that I could not refrain from cleaning that file up a little bit:

1. class ReaderWriterStateTSuite renamed to IndexedReaderWriterStateTSuite to make it corresponding to the file name.
2. "Organize imports" from VSCode.
3. The test for AlternativeLaws is made looking more alike other law tests around it.
4. Fixed compiler warnings (just in that file – yes, I did it there too!)

[satorg]

Nowadays, there's a lot of hype on AI technologies around – lots of people try to ruminate on what the AI can or cannot do and how it can be useful or even dangerous to all of us.

I personally think though, that at least one of the things that the AI could really help with – is to review such huge and boring PRs like this one.

[armanbilge]

is to review such huge and boring PRs like this one.

Sorry I dropped the ball on reviewing this 😢 also I think your force-pushes erased my review progress 😅

[satorg]

Hi, @armanbilge, @johnynek ,
Sorry for the ping.
Just to confirm with you – does it make sense to maintain this PR?
Actually, I'm not asking about rushing reviewing it,
but if you think it still could be useful (somewhen later),
then I could allocate some time to actualize it – rebase on main, resolve conflicts, etc.
Otherwise, I am fine just to go close it, no problem from my side – it does not look that critical anyway.
Thanks!