Add Align typeclass #3076

LukaJCB · 2019-09-20T18:21:35Z

Should fix #1263 while replacing #1755 by @julianmichael who did most of the work.

codecov-io · 2019-09-20T19:58:01Z

Codecov Report

Merging #3076 into master will increase coverage by 0.13%.
The diff coverage is 97.47%.

@@            Coverage Diff             @@
##           master    #3076      +/-   ##
==========================================
+ Coverage   93.17%   93.31%   +0.13%     
==========================================
  Files         372      375       +3     
  Lines        7182     7371     +189     
  Branches      199      215      +16     
==========================================
+ Hits         6692     6878     +186     
- Misses        490      493       +3

Flag	Coverage Δ
#scala_version_212	`93.64% <97.08%> (+0.11%)`	⬆️
#scala_version_213	`90.89% <94.11%> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
...in/scala/cats/data/AbstractNonEmptyInstances.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/data/Validated.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/NonEmptyList.scala	`99.35% <100%> (+0.02%)`	⬆️
...src/main/scala-2.13+/cats/instances/lazyList.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/either.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/Apply.scala	`78.94% <100%> (+3.94%)`	⬆️
core/src/main/scala/cats/data/NonEmptyChain.scala	`86.02% <100%> (+0.62%)`	⬆️
core/src/main/scala/cats/Align.scala	`100% <100%> (ø)`
core/src/main/scala/cats/SemigroupK.scala	`100% <100%> (ø)`	⬆️
.../main/scala-2.13+/cats/data/NonEmptyLazyList.scala	`62.5% <100%> (+1.63%)`	⬆️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 134570b...5be6a86. Read the comment docs.

…Apply

kailuowang · 2019-10-08T13:39:55Z

build.sbt

@@ -401,6 +401,9 @@ def mimaSettings(moduleName: String) =
          exclude[MissingClassProblem](
            "cats.kernel.compat.scalaVersionMoreSpecific$suppressUnusedImportWarningForScalaVersionMoreSpecific"
          )
+        ) ++ //abstract package private classes
+        Seq(
+          exclude[DirectMissingMethodProblem]("cats.data.AbstractNonEmptyInstances.this")


Are we sure this doesn't break BC? If we are sure maybe add a test in binCompatTest? If not can we just add a separate method to provide the instance for Align?

I added some tests there, I think that should suffice, WDYT?

I think that shall do. thanks!
On a slightly related note, since we have been updating the build, I am a bit nervous that if something breaks binCompatTest, we wouldn't know. I'll create an issue.

This just makes me vaguely uncomfortable. It's only used twice and it seems like it'd be almost equivalently noisy just to write out the new AbstractNonEmptyInstances[F, NEF] with Align[NEF] in those two places.

I know we don't promise bincompat for people using Java, etc., but breaking it for the sake of saving a few lines doesn't feel ideal.

core/src/main/scala-2.13+/cats/instances/lazyList.scala

core/src/main/scala/cats/Align.scala

core/src/main/scala/cats/instances/either.scala

kailuowang · 2019-10-15T16:35:58Z

just added some more additional comments. Sorry I couldn't find the chunk of time to review it in one go.

kailuowang

Thanks!

core/src/main/scala/cats/Align.scala

CremboC · 2019-10-22T18:57:25Z

Only partially related but I recently implemented very similar functionality (esp. padZipWith) for fs2.Stream:
https://gist.github.com/CremboC/e0515b5f15f17ee40a92659db78cbfd3

kailuowang · 2019-10-22T20:23:06Z

@djspiewak in my day-to-day use, I find it more convenient having leaf type classes encoded this way than inheritance. IMO the benefit outweight the benefit of having a uniform encoding for all type classes.

travisbrown

I still don't really like the "change our inheritance encoding as we go" approach but that ship sailed long ago I guess. 😄

I've got some nitpicks but nothing I see as an absolute blocker.

core/src/main/scala/cats/Align.scala

travisbrown · 2019-11-04T18:03:43Z

build.sbt

@@ -401,6 +401,9 @@ def mimaSettings(moduleName: String) =
          exclude[MissingClassProblem](
            "cats.kernel.compat.scalaVersionMoreSpecific$suppressUnusedImportWarningForScalaVersionMoreSpecific"
          )
+        ) ++ //abstract package private classes
+        Seq(
+          exclude[DirectMissingMethodProblem]("cats.data.AbstractNonEmptyInstances.this")


This just makes me vaguely uncomfortable. It's only used twice and it seems like it'd be almost equivalently noisy just to write out the new AbstractNonEmptyInstances[F, NEF] with Align[NEF] in those two places.

I know we don't promise bincompat for people using Java, etc., but breaking it for the sake of saving a few lines doesn't feel ideal.

LukaJCB · 2019-11-04T18:46:57Z

@travisbrown I addressed your points :)

travisbrown

@LukaJCB Looks good to me, thanks!

* backporting #3076 Unrelated changes * backporting #3076 Added Aligh typeclass * backporting #3076 Added Align typeclass * backporting #3076 Added Align instances Co-authored-by: Luka Jacobowitz <luka.jacobowitz@gmail.com>

LukaJCB added 2 commits September 20, 2019 14:20

Add Align typeclass

2523014

Scalafmt

909a33a

LukaJCB force-pushed the align branch from a1ca7fa to 909a33a Compare September 20, 2019 19:11

Add Const and Validated instances and derivation from SemigroupK and …

c5084bf

…Apply

LukaJCB force-pushed the align branch from 8a94d12 to c5084bf Compare September 25, 2019 17:01

Merge branch 'master' into align

fb6b0c6

LukaJCB requested a review from kailuowang September 25, 2019 20:01

kailuowang reviewed Oct 8, 2019

View reviewed changes

LukaJCB added 2 commits October 8, 2019 09:54

Merge branch 'master' into align

953a99f

Add MiMaException tests

006a23c

LukaJCB requested a review from kailuowang October 10, 2019 14:30

scalafmt

48eaa14

kailuowang reviewed Oct 11, 2019

View reviewed changes

core/src/main/scala-2.13+/cats/instances/lazyList.scala Outdated Show resolved Hide resolved

LukaJCB added 2 commits October 15, 2019 10:01

Merge branch 'master' into align

7a772b4

Use lazy iterator for align

e249b96

kailuowang reviewed Oct 15, 2019

View reviewed changes

core/src/main/scala/cats/Align.scala Outdated Show resolved Hide resolved

kailuowang reviewed Oct 15, 2019

View reviewed changes

core/src/main/scala/cats/instances/either.scala Show resolved Hide resolved

Add doctests and Either tests

6e7d2b4

kailuowang previously approved these changes Oct 18, 2019

View reviewed changes

djspiewak reviewed Oct 18, 2019

View reviewed changes

core/src/main/scala/cats/Align.scala Show resolved Hide resolved

LukaJCB requested a review from travisbrown November 4, 2019 17:28

travisbrown reviewed Nov 4, 2019

View reviewed changes

LukaJCB dismissed kailuowang’s stale review via e744414 November 4, 2019 18:46

LukaJCB requested review from kailuowang and travisbrown November 4, 2019 18:46

Address feedback

5be6a86

LukaJCB force-pushed the align branch from e744414 to 5be6a86 Compare November 4, 2019 21:43

travisbrown approved these changes Nov 5, 2019

View reviewed changes

LukaJCB merged commit 32a9e08 into master Nov 5, 2019

LukaJCB deleted the align branch November 5, 2019 09:55

travisbrown added the enhancement label Nov 6, 2019

travisbrown added this to the 2.1.0-RC1 milestone Nov 6, 2019

This was referenced Nov 13, 2019

Backport 2.1.0 changes for Scala 2.11 #3143

Closed

Add Align for Stream #3144

Merged

gagandeepkalra pushed a commit to gagandeepkalra/cats that referenced this pull request Mar 8, 2020

backporting typelevel#3076 Unrelated changes

cd10e01

gagandeepkalra added a commit to gagandeepkalra/cats that referenced this pull request Mar 8, 2020

backporting typelevel#3076 Added Aligh typeclass

5999702

gagandeepkalra added a commit to gagandeepkalra/cats that referenced this pull request Mar 8, 2020

backporting typelevel#3076 Added Align typeclass

e2c468b

gagandeepkalra added a commit to gagandeepkalra/cats that referenced this pull request Mar 8, 2020

backporting typelevel#3076 Added Align instances

56bb7d0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Align typeclass #3076

Add Align typeclass #3076

LukaJCB commented Sep 20, 2019 •

edited

Loading

codecov-io commented Sep 20, 2019 •

edited

Loading

kailuowang Oct 8, 2019 •

edited

Loading

LukaJCB Oct 10, 2019

kailuowang Oct 11, 2019

travisbrown Nov 4, 2019

kailuowang commented Oct 15, 2019

kailuowang left a comment

CremboC commented Oct 22, 2019

kailuowang commented Oct 22, 2019

travisbrown left a comment

travisbrown Nov 4, 2019

LukaJCB commented Nov 4, 2019

travisbrown left a comment

Add Align typeclass #3076

Add Align typeclass #3076

Conversation

LukaJCB commented Sep 20, 2019 • edited Loading

codecov-io commented Sep 20, 2019 • edited Loading

Codecov Report

kailuowang Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

LukaJCB Oct 10, 2019

Choose a reason for hiding this comment

kailuowang Oct 11, 2019

Choose a reason for hiding this comment

travisbrown Nov 4, 2019

Choose a reason for hiding this comment

kailuowang commented Oct 15, 2019

kailuowang left a comment

Choose a reason for hiding this comment

CremboC commented Oct 22, 2019

kailuowang commented Oct 22, 2019

travisbrown left a comment

Choose a reason for hiding this comment

travisbrown Nov 4, 2019

Choose a reason for hiding this comment

LukaJCB commented Nov 4, 2019

travisbrown left a comment

Choose a reason for hiding this comment

LukaJCB commented Sep 20, 2019 •

edited

Loading

codecov-io commented Sep 20, 2019 •

edited

Loading

kailuowang Oct 8, 2019 •

edited

Loading