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 IO Monad #109

Merged
merged 21 commits into from
Jul 16, 2017
Merged

Add IO Monad #109

merged 21 commits into from
Jul 16, 2017

Conversation

pakoito
Copy link
Member

@pakoito pakoito commented Jun 11, 2017

This PR adds an implementation of IO based off Cats. It includes a Duration util for blocking operations.

@codecov
Copy link

codecov bot commented Jun 11, 2017

Codecov Report

Merging #109 into master will decrease coverage by 2.05%.
The diff coverage is 36.55%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #109      +/-   ##
============================================
- Coverage     54.82%   52.77%   -2.06%     
- Complexity      177      183       +6     
============================================
  Files            77       83       +6     
  Lines          1098     1243     +145     
  Branches        169      180      +11     
============================================
+ Hits            602      656      +54     
- Misses          433      521      +88     
- Partials         63       66       +3
Impacted Files Coverage Δ Complexity Δ
katz/src/main/kotlin/katz/instances/IdMonad.kt 83.33% <ø> (ø) 0 <0> (ø) ⬇️
katz/src/main/kotlin/katz/instances/IOMonoid.kt 0% <0%> (ø) 0 <0> (?)
katz/src/main/kotlin/katz/instances/IOSemigroup.kt 0% <0%> (ø) 0 <0> (?)
katz/src/main/kotlin/katz/instances/IOMonad.kt 14.28% <14.28%> (ø) 0 <0> (?)
katz/src/main/kotlin/katz/effects/Duration.kt 27.77% <27.77%> (ø) 2 <2> (?)
katz/src/main/kotlin/katz/effects/IO.kt 40.9% <40.9%> (ø) 5 <5> (?)
...atz/src/main/kotlin/katz/effects/internal/Utils.kt 45.83% <45.83%> (ø) 0 <0> (?)
... and 4 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 7f10e83...a4d2965. Read the comment docs.

Copy link
Member

@ffgiraldez ffgiraldez left a comment

Choose a reason for hiding this comment

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

review IOMonad, IOMonoid and IOSemigrup to use IO<A> as parent generic type and returning type instead of HK<IO.F,A>

@@ -0,0 +1,14 @@
package katz

class IOMonoid<A>(val SM: Monoid<A>) : Monoid<HK<IO.F, A>> {
Copy link
Member

Choose a reason for hiding this comment

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

i think you could use IO<A> instead of HK definition here

package katz

class IOMonoid<A>(val SM: Monoid<A>) : Monoid<HK<IO.F, A>> {
override fun empty(): HK<IO.F, A> =
Copy link
Member

Choose a reason for hiding this comment

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

same here for returning type


fun <B> flatMap(f: (A) -> IO<B>): IO<B> =
flatMapTotal(
AndThen { a: A ->
Copy link
Member

Choose a reason for hiding this comment

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

it's necessary specify the Type here?


companion object {
// Actually limited to 9223372036854775807 days, so unless you are very patient, it is unlimited ;-)
val Infinite = Duration(amount = Long.MAX_VALUE, timeUnit = TimeUnit.DAYS)
Copy link
Member

Choose a reason for hiding this comment

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

Is the constants naming style capitalized?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

Copy link
Member

@raulraja raulraja left a comment

Choose a reason for hiding this comment

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

Looks great but we need to change those methods returning Option in the unsafe* runners.

@@ -0,0 +1,70 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

We should have attribution in files like License or where they belong like we do in the typelevel cats case.

@@ -0,0 +1,11 @@
package katz

class IOSemigroup<A>(val SM: Semigroup<A>) : Semigroup<HK<IO.F, A>> {
Copy link
Member

Choose a reason for hiding this comment

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

should this use default params for the registered instance value if not provided at instantiation?

Copy link
Member Author

Choose a reason for hiding this comment

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

The companion object has invoke overloaded to provide a default value. n this case IO acts as a wrapper, and the SemiGroup depends mostly on the contents. We could have separate invokes for common types that are not globally registered like numbers and lists.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense

}
}

override fun unsafeRunTimedTotal(limit: Duration): Option<A> =
Copy link
Member

Choose a reason for hiding this comment

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

I think returning Option in any of the unsafe methods will be confusing to users. We should return the expected value and since those methods are already unsafe let any exceptions propagate if unfolding the Option returned a NoSuchElementException.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason why this return Option is that None represents executing code that returns null. Errors still throw. I don't believe we should stop null-capable execution so we have to choose between returning Option or A?

Copy link
Member Author

Choose a reason for hiding this comment

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

We've had a chat and decided on the API. unsafeRunTimedTotal returns Option, unsafeRun() returns A.

}
}

val Long.days: Duration
Copy link
Member

Choose a reason for hiding this comment

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

interesting way to provide duration out from any numeric types.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's copypasterino, check the license atop of the file :D

Copy link
Member

Choose a reason for hiding this comment

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

Ok heheheh. Didn't see it.
image

package katz.effects.internal

/**
* Abandon all hope
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some docs here about the purpose of this class ? I am trying to learn something here and that could be helpful :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in the branch that adds AndThen!

pakoito added 7 commits June 12, 2017 21:43
@codecov-io
Copy link

codecov-io commented Jul 16, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@d1c8aa2). Click here to learn what that means.
The diff coverage is 50.33%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #109   +/-   ##
========================================
  Coverage          ?   55.2%           
  Complexity        ?     212           
========================================
  Files             ?      86           
  Lines             ?    1317           
  Branches          ?     179           
========================================
  Hits              ?     727           
  Misses            ?     519           
  Partials          ?      71
Impacted Files Coverage Δ Complexity Δ
...gory/src/main/kotlin/kategory/instances/IdMonad.kt 71.42% <ø> (ø) 0 <0> (?)
.../src/main/kotlin/kategory/instances/StateTMonad.kt 0% <0%> (ø) 0 <0> (?)
.../src/main/kotlin/kategory/instances/IorTraverse.kt 0% <0%> (ø) 0 <0> (?)
...egory/src/main/kotlin/kategory/effects/Duration.kt 27.77% <27.77%> (ø) 2 <2> (?)
...src/main/kotlin/kategory/effects/internal/Utils.kt 45.83% <45.83%> (ø) 0 <0> (?)
kategory/src/main/kotlin/kategory/effects/IO.kt 55.68% <55.68%> (ø) 7 <7> (?)
...gory/src/main/kotlin/kategory/instances/IOMonad.kt 57.14% <57.14%> (ø) 0 <0> (?)
...ory/src/main/kotlin/kategory/instances/IOMonoid.kt 75% <75%> (ø) 2 <2> (?)
.../src/main/kotlin/kategory/instances/IOSemigroup.kt 75% <75%> (ø) 1 <1> (?)

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 d1c8aa2...f302179. Read the comment docs.

@ffgiraldez ffgiraldez merged commit 1f1b1cc into master Jul 16, 2017
@ffgiraldez ffgiraldez deleted the paco-ayo branch July 16, 2017 14:40
@wiyarmir wiyarmir mentioned this pull request Jul 17, 2017
ambrusadrianz pushed a commit to ambrusadrianz/arrow that referenced this pull request Oct 22, 2019
rachelcarmena added a commit that referenced this pull request Feb 24, 2021
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.

5 participants