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

Syntax for function1 kleisli-composition #3396

Merged
merged 6 commits into from
Jun 16, 2020

Conversation

valenterry
Copy link
Contributor

@valenterry valenterry commented Apr 16, 2020

This PR aims to add syntax for easily composing functions without wrapping them into Kleisli manually, based on a gitter conversation: https://gitter.im/typelevel/cats?at=5e971c478821056d3e124e4a (@Daenyth @LukaJCB)

I looked into the tests, but I'm unsure what tests to write for the syntax methods that are added in this PR. Optimally I would like to write a property test / law in the style of forAll{ f >=> g <-> Kleisli(f) >>> Kleisli(g) }

TODO

  • Add tests for the new syntax methods (doctest)

@LukaJCB
Copy link
Member

LukaJCB commented Apr 16, 2020

We could skip the wrapping in Kleisli by only requiring FlatMap[F]

@LukaJCB
Copy link
Member

LukaJCB commented Apr 16, 2020

In terms of test, I think some simple doctests would be enough I think (along with the syntax tests you already wrote) :)

@codecov-io
Copy link

codecov-io commented Apr 17, 2020

Codecov Report

Merging #3396 into master will increase coverage by 0.17%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3396      +/-   ##
==========================================
+ Coverage   92.49%   92.67%   +0.17%     
==========================================
  Files         378      379       +1     
  Lines        7956     7984      +28     
  Branches      227      230       +3     
==========================================
+ Hits         7359     7399      +40     
+ Misses        597      585      -12     
Flag Coverage Δ
#scala_version_212 92.67% <0.00%> (?)
#scala_version_213 92.46% <0.00%> (-0.04%) ⬇️
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/function1.scala 40.00% <0.00%> (-60.00%) ⬇️
core/src/main/scala/cats/data/Const.scala 100.00% <0.00%> (ø)
core/src/main/scala/cats/data/Validated.scala 100.00% <0.00%> (ø)
core/src/main/scala/cats/instances/tuple.scala 100.00% <0.00%> (ø)
core/src/main/scala/cats/instances/option.scala 100.00% <0.00%> (ø)
...cala-2.12/cats/kernel/compat/TraversableOnce.scala 0.00% <0.00%> (ø)
core/src/main/scala/cats/data/NonEmptyList.scala 98.73% <0.00%> (+0.01%) ⬆️
core/src/main/scala/cats/data/NonEmptyVector.scala 99.26% <0.00%> (+0.01%) ⬆️
core/src/main/scala/cats/data/NonEmptySet.scala 97.64% <0.00%> (+0.02%) ⬆️
...ore/src/main/scala/cats/data/NonEmptyMapImpl.scala 97.50% <0.00%> (+0.03%) ⬆️
... and 10 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 9e02ec0...0426d46. Read the comment docs.

@valenterry valenterry changed the title Syntax for function1 kleisli-composition [help for tests appreciated] Syntax for function1 kleisli-composition Apr 17, 2020
@valenterry valenterry marked this pull request as ready for review April 17, 2020 06:37
@valenterry
Copy link
Contributor Author

In terms of test, I think some simple doctests would be enough I think (along with the syntax tests you already wrote) :)

Addressed that and changed it to use FlatMap constraint

* res2: Option[Int] = None
* }}}
*/
def >=>[C](g: B => F[C]): A => F[C] = a => FlatMap[F].flatMap(f(a))(g)
Copy link
Contributor Author

@valenterry valenterry Apr 17, 2020

Choose a reason for hiding this comment

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

Should we @inline this?

Copy link
Contributor

Choose a reason for hiding this comment

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

my two cents here, the other syntax ops are not inlined in general so I wouldn't do it.

LukaJCB
LukaJCB previously approved these changes Apr 28, 2020
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

thanks!

@LukaJCB LukaJCB requested a review from kailuowang May 12, 2020 15:54
@bplommer
Copy link
Contributor

I think this would benefit from also having non-symbolic names - maybe andThenF / composeF?

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.

In Cats we've been leaning towards having non-symbolic names (with symbolic alias) in most if not all cases. Can we stick to that convention here too?

@valenterry
Copy link
Contributor Author

In Cats we've been leaning towards having non-symbolic names (with symbolic alias) in most if not all cases. Can we stick to that convention here too?

Sure! Any preference for the method name? andThenF makes sense to me or andThenK (which was/is used in scalaz AFAIR)?

@LukaJCB
Copy link
Member

LukaJCB commented May 28, 2020

I like composeF and andThenF :)

@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2020

Codecov Report

Merging #3396 into master will decrease coverage by 0.88%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #3396      +/-   ##
==========================================
- Coverage   92.49%   91.60%   -0.89%     
==========================================
  Files         378      382       +4     
  Lines        7956     8307     +351     
  Branches      227      218       -9     
==========================================
+ Hits         7359     7610     +251     
- Misses        597      697     +100     

@LukaJCB LukaJCB merged commit 36094e7 into typelevel:master Jun 16, 2020
@valenterry valenterry deleted the function1-compose-kleisli-syntax branch June 16, 2020 16:08
@travisbrown travisbrown added this to the 2.2.0-M3 milestone Jun 17, 2020
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.

8 participants