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.

Sign up for GitHub

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

Jump to bottom

Improvements to Inject. #1557

Merged
merged 5 commits into from
Mar 20, 2017
Merged

Improvements to Inject. #1557

merged 5 commits into from
Mar 20, 2017

Conversation

sellout
Copy link
Contributor

@sellout sellout commented Mar 13, 2017 •
edited
Loading

  • move it back to cats-core (near Coproduct)
  • make inj/prj natural transformations
  • add apply/unapply (lowered inj/prj)
  • define Free.roll
  • move injectRoll (née Inject.inject) and match_ to Free
  • reintroduce "null identity" test (written by @edmundnoble)

@sellout
Improvements to Inject.
003141e 
- move it back to cats-core (near `Coproduct`)
- make `inj`/`prj` natural transformations
- add `apply`/`unapply` (lowered `inj`/`prj`)
- define `Free.roll`
- move `injectRoll` (née `Inject.inject`) and `match_` to `Free`
- reintroduce "null identity" test (written by @edmundnoble)
@sellout
Copy link
Contributor Author

sellout commented Mar 13, 2017

These are largely changes needed to ease Quasar’s migration.

adelbertc
adelbertc approved these changes Mar 13, 2017
View reviewed changes
tests/src/test/scala/cats/tests/InjectTests.scala Outdated
@@ -0,0 +1,81 @@
package cats

// import cats.arrow.FunctionK
Copy link
Contributor

Choose a reason for hiding this comment

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

woops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I apparently broke tut. Ok, fixes in the morning 😅

core/src/main/scala/cats/Inject.scala
sealed abstract class Inject[F[_], G[_]] {
def inj: FunctionK[F, G]

def prj: FunctionK[G, λ[α => Option[F[α]]]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call these inject and project ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m fine with either. The current ones match the paper and Scalaz. Also, Matryoshka defines project, so I don’t mind the lack of collision 😁

@notxcain
Copy link
Contributor

@sellout did you see #1505 in the wild? If so, how do you workaround?

@sellout
Copy link
Contributor Author

sellout commented Mar 13, 2017

@notxcain Sorry, I haven’t seen that.

kailuowang
kailuowang reviewed Mar 14, 2017
View reviewed changes
core/src/main/scala/cats/Inject.scala Outdated

import cats.arrow.FunctionK
import cats.data.Coproduct
import cats.syntax.either._
Copy link
Contributor

Choose a reason for hiding this comment

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

unused import here breaking the build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I have a false memory of seeing a green build, so this would have sat for a while before I noticed.

@codecov-io
Copy link

codecov-io commented Mar 14, 2017 •
edited
Loading

Codecov Report

Merging #1557 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1557      +/-   ##
=========================================
+ Coverage   92.48%   92.5%   +0.01%     
=========================================
  Files         246     246              
  Lines        3886    3895       +9     
  Branches      132     134       +2     
=========================================
+ Hits         3594    3603       +9     
  Misses        292     292
Impacted Files Coverage Δ
core/src/main/scala/cats/package.scala 100% <ø> (ø) ✅
core/src/main/scala/cats/Inject.scala 100% <100%> (ø)
free/src/main/scala/cats/free/Free.scala 88.23% <100%> (+0.73%) ✅
core/src/main/scala/cats/Eval.scala 97.29% <0%> (+0.19%) ✅

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 383e0c4...380ea81. Read the comment docs.

@kailuowang
Copy link
Contributor

Sorry my bad. The either syntax is used in scala 2.11 and 2.10 withi this toOption. It's unused in Scala 2.12 because Either is right biased and toOption runs on the right projection. So now the build on 2.10 and 2.11 is broken. A fix off top of my head is to write _.run.swap.right.toOption instead. There may be a better way to support the different syntax in different Scala? cc @adelbertc

@kailuowang kailuowang closed this Mar 15, 2017
@kailuowang kailuowang reopened this Mar 15, 2017
@sellout
Copy link
Contributor Author

sellout commented Mar 15, 2017

@kailuowang Yeah – I brought this up on the gitter channel. @paulp mentioned ghik/silencer, but that doesn’t support 2.10. I’ll try the swap approach. Thanks.

sellout added 2 commits March 16, 2017 15:26
@sellout
Change to not require syntax.either.
baeec37 
Otherwise we need it for 2.10 & 2.11 and need to _not_ have it for 2.12.
@sellout
Add tests for Inject#apply and #unapply.
380ea81
@kailuowang
Copy link
Contributor

👍 LGTM, since this is a breaking change, we might want to wait for a third maintainer sign-off.

peterneyens
peterneyens approved these changes Mar 18, 2017
View reviewed changes
Copy link
Collaborator

@peterneyens peterneyens left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks also for the inject / injectRoll improvements.

@kailuowang
Copy link
Contributor

merging with three sign-offs

@kailuowang kailuowang merged commit 8fbf546 into typelevel:master Mar 20, 2017
@kailuowang kailuowang mentioned this pull request Mar 27, 2017
Merged
@kailuowang kailuowang modified the milestone: 1.0.0-MF Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kailuowang kailuowang kailuowang left review comments

@peterneyens peterneyens peterneyens approved these changes

@adelbertc adelbertc adelbertc approved these changes

Assignees
No one assigned
Labels
Source Breaking
Projects
None yet
Milestone
1.0.0-MF
Development

Successfully merging this pull request may close these issues.
6 participants
@sellout @notxcain @codecov-io @kailuowang @adelbertc @peterneyens