Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Align typeclass #3076

Merged
merged 11 commits into from
Nov 5, 2019
Merged

Add Align typeclass #3076

merged 11 commits into from
Nov 5, 2019

Conversation

LukaJCB
Copy link
Member

@LukaJCB LukaJCB commented Sep 20, 2019

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

@codecov-io
Copy link

codecov-io commented Sep 20, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

build.sbt Outdated
@@ -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")
Copy link
Contributor

@kailuowang kailuowang Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kailuowang
Copy link
Contributor

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

kailuowang
kailuowang previously approved these changes Oct 18, 2019
Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@CremboC
Copy link

CremboC commented Oct 22, 2019

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

@kailuowang
Copy link
Contributor

@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.

Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
core/src/main/scala/cats/Align.scala Outdated Show resolved Hide resolved
build.sbt Outdated
@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

LukaJCB commented Nov 4, 2019

@travisbrown I addressed your points :)

Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LukaJCB Looks good to me, thanks!

@LukaJCB LukaJCB merged commit 32a9e08 into master Nov 5, 2019
@LukaJCB LukaJCB deleted the align branch November 5, 2019 09:55
@travisbrown travisbrown added this to the 2.1.0-RC1 milestone Nov 6, 2019
gagandeepkalra pushed a commit to gagandeepkalra/cats that referenced this pull request Mar 8, 2020
gagandeepkalra added a commit to gagandeepkalra/cats that referenced this pull request Mar 8, 2020
gagandeepkalra added a commit to gagandeepkalra/cats that referenced this pull request Mar 8, 2020
gagandeepkalra added a commit to gagandeepkalra/cats that referenced this pull request Mar 8, 2020
travisbrown pushed a commit that referenced this pull request Mar 11, 2020
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align type class
6 participants