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
Apply typeclass #1404
Apply typeclass #1404
Conversation
…ss how many extensions we will add
Finally, I had to rever the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far!
@extension | ||
interface TryApply : Apply<ForTry> { | ||
override fun <A, B> TryOf<A>.ap(ff: TryOf<(A) -> B>): Try<B> = | ||
fix().ap(ff) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe in Arrow we use 2 spaces but we are in the process of adding Ktlint // cc @aballano . You can leave or change this but eventually, it's gonna get reformated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, I couldn't find any tool to automatically format the code and I decided to left the formatting issue as is until I get the answers about the docs and the extensions 😃 I'll format the code properly before finishing this PR. Don't worry 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still working on it! #1405
import arrow.core.* | ||
import java.math.BigDecimal | ||
|
||
interface Apply<F> : Functor<F> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'll never need to open this file anymore.
|
||
object ApplyLaws { | ||
|
||
fun <F, A> laws(AP: Applicative<F>, gen: Gen<A>, EQ: Eq<Kind<F, A>>): List<Law> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep the laws separated between those that use Apply and those that use Applicative.
Then I realised all of them needed both lol
I'd keep them as ApplicativeLaws tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But do they? If just is only used to lift into Kind<F, A>
then it might be better to either use a constructor function or a Gen<Kind<F, A>>
directly. And all laws that strictly need just belong to applicative anyway. Although I am not too familiar with applicative laws, so idk how many that leaves o.O
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an issue we have in more places.
We should indeed be using Gen<Kind<F, A>>
directly here. There is some low hanging fruit in the test setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi all, I commented one interesting point about the laws in the PR description:
It was weird to see how the laws already described for Aplicative doesn't match with the one I've found in other implementations like Cat's Apply or Scalaz's Apply typeclasses.
I've updated the AplicativeLaws implementation to be divided in ApplyLaws and ApplyCartesianLaws. This change let us write some of our laws without coupling the implementation to any concrete type. I'm afraid I couldn't implement this for the cartesian laws because of the internal map implemented. Ideally, we'd like to don't even couple this implementation to the Applicative instance passed as parameter, but there is no way to lift any instance into Kind<F, A> w/o an Applicative instance. I've reviewed the laws implementation with @delr3ves thinking about how could reduce the coupling but it wasn't possible for this scenario.
Finally, as FunctorLaws
need some dependencies I can't fix for now without rewriting most of the laws and this issue was supposed to bee "noob friendly" I decided not to refactor them and just move them to the ApplyLaw
folder.
Are we going to keep the laws in the ApplicativeLaw
folder and remove this part of the PR? I tried to use Gen<Kind<F, A>>, and a method to lift any value into Kind, but it is not so easy without refactoring the rest of the code laws.
|
||
import arrow.Kind | ||
import arrow.core.* | ||
import java.math.BigDecimal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import seems weird, is it used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I have pending the imports optimization and the code formatting for this PR.
Looks good so far 👍 I'd be in favor of adding the instances as it could be confusing not having them. Although I don't think it is super important, but at least a ticket for adding them should be added |
Thanks for your comments guys! Do we all agree if we finish the following task before considering this PR as final?
|
Why not a new docs page? |
I asked for this in the PR description @pakoito
As the usage of Let me know if you'd prefer a new page or just a section inside applicative and I'll update the PR with it. |
Hi there! As discussed in this draft PR I've updated this PR with the extensions needed, the documentation updated and a new section added to the applicative documentation about the Apply typeclass 😃 So I'm moving this PR to the final stage. I'd like to thank you all for your help and comments. Please, let me know if you think there is anything else you'd like to add or review 😃 |
modules/core/arrow-core-extensions/src/main/kotlin/arrow/core/extensions/try.kt
Outdated
Show resolved
Hide resolved
modules/core/arrow-test/src/main/kotlin/arrow/test/laws/ApplicativeLaws.kt
Outdated
Show resolved
Hide resolved
modules/core/arrow-typeclasses/src/main/kotlin/arrow/typeclasses/Applicative.kt
Show resolved
Hide resolved
@@ -157,7 +157,7 @@ binding { | |||
``` | |||
|
|||
```kotlin:ank | |||
import arrow.core.extensions.option.applicative.map | |||
import arrow.core.extensions.option.apply.map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have we found collision issues with the stdlib apply
function when requesting the apply()
instance explicitly from a companion? Seems like this could be potentially confusing to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope not. I know the name can be confusing for some users but as most of the people will consume it as is in this example I guess this won't be an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raulraja there is no issue with apply
. It's called invoke
in Kotlin. Scala mix-up :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nomisRev I think @raulraja is talking about this one:
package kotlin
/**
* Calls the specified function [block] with `this` value as its receiver and returns `this` value.
*/
@kotlin.internal.InlineOnly
public inline fun <T> T.apply(block: T.() -> Unit): T {
contract {
callsInPlace(block, InvocationKind.EXACTLY_ONCE)
}
block()
return this
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this can go as is it's missing something very important. Kdocs. The docs in markdown are deprecated and we want all examples and playgrounds ready as part of the Kdocs. See Functor
for an example on how that is done. Also relevant https://github.com/arrow-kt/arrow/blob/master/modules/docs/arrow-docs/README.md
We want to abandon md based docs in favor of kdocs.
modules/docs/arrow-docs/docs/docs/arrow/typeclasses/applicative/README.md
Outdated
Show resolved
Hide resolved
Awesome stuff, thanks @pedrovgs ! 👏 |
Thank you all for your comments and feedback. I'll keep contributing to this awesome project when possible 😃 |
📌 References
🎩 What is the goal?
Add
Apply
typeclass to the project and updateApplicative
typeclass implementation to be based on it.Disclaimer: I'm creating this PR as a draft because the implementation is not finished. There are some pending tasks related to the extensions and documentation I'd like to review with the team before. Formatting the code properly is also a task I have pending but I can't spend more time on this issue for now.
How is it being implemented?
Crating the brand new
Apply
typeclass was easy. We only needed to create the newApply
type and move all the code fromApplicative
but thejust
related code. The hard part was related to the laws and the usage of simulated higher kinded types.These are some points I'd like to review with the team:
Aplicative
doesn't match with the one I've found in other implementations like Cat's Apply or Scalaz's Apply typeclasses.AplicativeLaws
implementation to be divided inApplyLaws
andApplyCartesianLaws
. This change let us write some of our laws without coupling the implementation to any concrete type. I'm afraid I couldn't implement this for the cartesian laws because of the internal map implemented. Ideally, we'd like to don't even couple this implementation to theApplicative
instance passed as parameter, but there is no way to lift any instance intoKind<F, A>
w/o anApplicative
instance. I've reviewed the laws implementation with @delr3ves thinking about how could reduce the coupling but it wasn't possible for this scenario.About the rest of the task. I have pending to write the documentation and implement the extensions for the types already providing an
Applicative
instance. However I have questions about these tasks:Apply
typeclass is it ok if we just update theApplicative
page?Apply
is only needed to be able to implement other typeclasses for now, is adding all the extensions worth it?Please, review the current implementation and my questions about the docs and the extensions. As soon as we consider this implementation correct I'll update the PR with the rest of the code 😃
How can it be tested?
🤖 All the code already using the
ApplicativeLaws
will now use theApplyLaws
.