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

Improving the FpToTheMax example #997

Merged
merged 6 commits into from
Aug 28, 2018
Merged

Improving the FpToTheMax example #997

merged 6 commits into from
Aug 28, 2018

Conversation

enhan
Copy link
Contributor

@enhan enhan commented Aug 24, 2018

I just added TestIO based on the State datatype & included the version of @raulraja .

@codecov
Copy link

codecov bot commented Aug 24, 2018

Codecov Report

Merging #997 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #997      +/-   ##
============================================
- Coverage     46.95%   46.94%   -0.02%     
  Complexity      688      688              
============================================
  Files           318      318              
  Lines          8073     8073              
  Branches        848      848              
============================================
- Hits           3791     3790       -1     
  Misses         3940     3940              
- Partials        342      343       +1
Impacted Files Coverage Δ Complexity Δ
...tics/src/main/kotlin/arrow/optics/instances/map.kt 89.18% <0%> (-2.71%) 0% <0%> (ø)

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 22f63f5...9898f3c. Read the comment docs.

@nfrankel
Copy link
Contributor

As a newbie, I don't understand the need to wrap system calls in IO to "unwrap" them later with bind()?

IO { println("foo") }.bind()

It might be necessary in Scala, but it seems pretty purposeless in Kotlin.

@raulraja
Copy link
Member

Both println and readLine perform effects. println causes the external effect that when applied something gets printed to a log or system.out. readLine causes the effect that regardless of the input parameters you get always something different back therefore introduces the same kind of effect you would have if you had a random number generator in your computation. They both make functions impure since you can’t expect the function to produce the same output for a given input without either modifying the external world or making it non deterministic.
For that reason effects are encapsulated with either IO or if you are using type classes anything above MonadDefer.
Proof that these effects happen is:

fun Applicative<F>.printOpImpure: Kind<F, Unit> = pure(println("whatever")) 
// because pure is strict println gets evaluated each time `printOpImpure` is invoked and can't be part of the final IO loop that controls effects and exceptions. In your example it will be similar. If there were no previous bindings in the computation 
fun MonadDefer<F>.printOpPure: Kind<F, Unit> =  invoke { println("whatever") } 
// because `invoke` in MonadDefer is lazy invoking `printOpPure` will not cause printing to happen and the effect will be treated along the rest of computation when executing the `program.unsafeRunSync` at the edge```
(edited)

A different question is whether we can leave logging out of the effects we care to track.
To summarize, anything that causes a change to the external world or makes a function impure as a rule of thumb should be encapsulated in IO#invoke or suspended with a type class like MonadDefer#invoke.

In regards to Scala is the same as Kotlin, both langs allow devs to perform effect without being in IO.

@pakoito
Copy link
Member

pakoito commented Aug 27, 2018

Wrap, then merge!

@enhan
Copy link
Contributor Author

enhan commented Aug 27, 2018

Hi @pakoito , what do you mean by wrap ? Should I squash the commits ?

@pakoito
Copy link
Member

pakoito commented Aug 28, 2018

@enhan Replace those read/println().liftIO() with IO { read/println() } to make them pure, and we'll merge afterwards!

@enhan
Copy link
Contributor Author

enhan commented Aug 28, 2018

@pakoito I think there is no liftIO used in this PR ! @nfrankel used it in its own implementation of FP to the Max, if I'm not mistaking.

@pakoito
Copy link
Member

pakoito commented Aug 28, 2018

I see! I'll merge as soon as CI is ready, then

@enhan
Copy link
Contributor Author

enhan commented Aug 28, 2018

nice :) !

@codecov
Copy link

codecov bot commented Aug 28, 2018

Codecov Report

Merging #997 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #997      +/-   ##
============================================
+ Coverage     47.04%   47.05%   +0.01%     
  Complexity      694      694              
============================================
  Files           320      320              
  Lines          8090     8090              
  Branches        848      848              
============================================
+ Hits           3806     3807       +1     
  Misses         3941     3941              
+ Partials        343      342       -1
Impacted Files Coverage Δ Complexity Δ
...ics/src/main/kotlin/arrow/optics/instances/mapk.kt 90.9% <0%> (+4.54%) 0% <0%> (ø) ⬇️

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 ecdf60b...e2e220f. Read the comment docs.

@pakoito pakoito merged commit e8e2adc into arrow-kt:master Aug 28, 2018
@raulraja raulraja mentioned this pull request Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants