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

Optics state API #1047

Merged
merged 15 commits into from Oct 21, 2018

Conversation

Projects
None yet
3 participants
@nomisRev
Member

nomisRev commented Oct 13, 2018

No description provided.

@codecov

This comment has been minimized.

codecov bot commented Oct 13, 2018

Codecov Report

Merging #1047 into master will increase coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1047      +/-   ##
============================================
+ Coverage     42.29%   42.52%   +0.23%     
  Complexity      737      737              
============================================
  Files           353      353              
  Lines          9715     9749      +34     
  Branches       1056     1056              
============================================
+ Hits           4109     4146      +37     
+ Misses         5293     5290       -3     
  Partials        313      313
Impacted Files Coverage Δ Complexity Δ
.../arrow-optics/src/main/kotlin/arrow/optics/Lens.kt 78.2% <ø> (+3.96%) 0 <0> (ø) ⬇️
...rrow-optics/src/main/kotlin/arrow/optics/Getter.kt 63.15% <100%> (+3.15%) 0 <0> (ø) ⬇️
...ow-optics/src/main/kotlin/arrow/optics/Optional.kt 78.2% <100%> (+2.84%) 0 <0> (ø) ⬇️
...rrow-optics/src/main/kotlin/arrow/optics/Setter.kt 55.55% <100%> (+9.4%) 0 <0> (ø) ⬇️
...w-optics/src/main/kotlin/arrow/optics/Traversal.kt 68.42% <100%> (+3.3%) 0 <0> (ø) ⬇️
...ore/arrow-data/src/main/kotlin/arrow/data/State.kt 50% <0%> (+5%) 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 4d3fbb5...1d982cd. Read the comment docs.

/**
* Modify the focus [A] viewed through the [Optional] and returns its *old* value.
*/
fun <S, A> Optional<S, A>.modo(f: (A) -> A): State<S, Option<A>> =

This comment has been minimized.

@pakoito

pakoito Oct 13, 2018

Member

We can have modify, modifyOld, assign, assignOld. In this case longer names will be helpful.

This comment has been minimized.

@nomisRev

nomisRev Oct 13, 2018

Member

Most Optics already have a method modify so this would conflict. I was not a big fan of modifyState & co but am open to suggestions!

This comment has been minimized.

@pakoito

pakoito Oct 13, 2018

Member

update then

/**
* Extracts the focus [A] viewed through the [POptional] and applies [f] to it.
*/
fun <C> extracts(f: (A) -> C): State<S, Option<C>> = extract().map { it.map(f) }

This comment has been minimized.

@pakoito

pakoito Oct 13, 2018

Member

This should be called extractMap.

This comment has been minimized.

@nomisRev

nomisRev Oct 13, 2018

Member

I'll make this change.

@nomisRev

This comment has been minimized.

Member

nomisRev commented Oct 13, 2018

@pakoito & @raulraja also the mod & assign variants are now forced to monomorphic optics since we lack https://github.com/nomisRev/kategory/blob/simon-dontfeartheoptics-wip/kategory-core/src/main/kotlin/kategory/data/IndexedStateT.kt

polymoprhic version

But we lose type safety in binding with IndexedStateT. Not sure if we have a solution for that now. And/or if we're interested in having IndexedStateT despite this short? Can provide example show casing type safety issue if wanted.

@raulraja

This comment has been minimized.

Member

raulraja commented Oct 13, 2018

We can bring IndexedStateT back if we need it to bring polymorphism back to mod and assign. I don't recall what the problem with binding was. We can potentially make it a private data type in optics that does not explicitly adhere to Monad and therefore does not inherit binding.

@nomisRev

This comment has been minimized.

Member

nomisRev commented Oct 13, 2018

IndexedStateT was never merged. I never created a PR because the compiler was tripping over the generics in flatMap but that seems to be fixed.

sealed class DoorState
object Open: DoorState()
object Closed: DoorState()

val open: IndexedStateT<EvalHK, Closed, Open, Unit> = IndexedStateT.set(Eval.applicative(), Open)
val close: IndexedStateT<EvalHK, Open, Closed, Unit> = IndexedStateT.set(Eval.applicative(), Closed)

open.flatMap(Eval.monad()) { _ ->
  close.flatMap(Eval.monad()) { _ ->
    open
  }
}.run(Closed, Eval.monad())
  .ev()
  .value()
  .let(::println)  //Tuple2(a=kategory.Open@2a84aee7, b=kotlin.Unit)


open.flatMap(Eval.monad()) { _ ->
  close.flatMap(Eval.monad()) { _ ->
    close //Required: (Unit) -> IndexedStateT<EvalHK, Closed, SB, A> but found (Unit) -> IndexedStateT<EvalHK, Open, Closed, Unit>
  }
}.run(Closed, Eval.monad())
//^ Doesn't compile
//Error:(26, 33) Kotlin: Type mismatch: inferred type is (Unit) -> IndexedStateT<EvalHK, Open, Closed, Unit> but (Unit) -> IndexedStateTKind<EvalHK, Closed, Closed, Unit> /* = HK<HK3<[ERROR : Unknown type parameter 7], [ERROR : Unknown type parameter 8], [ERROR : Unknown type parameter 9], [ERROR : Unknown type parameter 10]> /* = HK<HK2<[ERROR : Unknown type parameter 4], [ERROR : Unknown type parameter 5], [ERROR : Unknown type parameter 6]> /* = HK<HK<IndexedStateTHK, EvalHK>, Closed> */, Closed> */, Unit> */ was expected

This is the current state and it seems to be working great! So I could actually clean it up and write some documentation for it and create a PR. Cleaning up the code shouldn't be too hard and I seemed to have added KDoc for everything already 2.

I am not sure what I am remembering about the binding but it seems to be fine although not really usefull. The monad instance fixes to IndexedStateT<F, S, S, A> but that is the case 2 in Cats.

@nomisRev

This comment has been minimized.

Member

nomisRev commented Oct 18, 2018

@pakoito updated :D

@raulraja

This is great Simon!. In 0.8.0 we are gonna have syntax directly also over List and ListK will only be needed when using List in a polymorphic function that requires the value to be in kind position.

@raulraja

This comment has been minimized.

Member

raulraja commented Oct 19, 2018

Let's wait until #1040 is merged first please

@raulraja

This comment has been minimized.

Member

raulraja commented Oct 19, 2018

@nomisRev This is ready to go once you rebase from master

@nomisRev nomisRev force-pushed the simon-optics-state branch from 3cfa496 to 132a787 Oct 21, 2018

@nomisRev nomisRev merged commit cff7024 into master Oct 21, 2018

4 checks passed

codecov/patch 100% of diff hit (target 42.29%)
Details
codecov/project 42.52% (+0.23%) compared to 4d3fbb5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@nomisRev nomisRev deleted the simon-optics-state branch Oct 21, 2018

@raulraja raulraja referenced this pull request Nov 2, 2018

Merged

Release 0.8.0 #1080

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment