Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

Bugfix/applicative both (fixes #3) #6

Merged
merged 2 commits into from
Sep 9, 2018
Merged

Bugfix/applicative both (fixes #3) #6

merged 2 commits into from
Sep 9, 2018

Conversation

texastoland
Copy link
Contributor

@texastoland texastoland commented Sep 8, 2018

both in ppx_let is for concurrency but the current implementation forces sequential execution.

Test

A concurrent applicative needs lazy evaluation. Since OCaml is strict the monad's type is a simple callback to the underlying type. The methods are straightforward except apply which awaits both callbacks concurrently. If its state is empty it stores the underlying value otherwise it applies both for the listener. bind's tests expect callbacks to execute sequentially while the both's verify concurrency.

Implementation

Adds apply_both to the Apply type class. It seemed perfect because of its symmetry to apply_first. Formally both is the Monoidal equivalent of apply (commonly called merge). I thought both would need pure for lifted map but it worked without it. I also considered supplying overrides but it's tricky to implement lawfully.

`both` in `ppx_let` is for concurrency but the current implementation forces sequential execution. A concurrent applicative needs lazy evaluation. Since OCaml is strict the monad's type is a simple callback to the underlying type. The methods are straightforward except `apply` which awaits both callbacks concurrently. If its state is empty it stores the underlying value otherwise it applies both for the listener. The `bind` tests expect callbacks to execute sequentially while the `both` tests verify concurrency.
Adds `apply_both` to the `Apply` type class. It seemed perfect because of its symmetry to `apply_first`. Formally `both` is the `Monoidal` equivalent of `apply`. I thought `both` would need `pure` for lifted `map` but it worked without it. I also considered supplying overrides but it's tricky to implement lawfully.
@Risto-Stevcev Risto-Stevcev merged commit 00a3831 into Risto-Stevcev:master Sep 9, 2018
@Risto-Stevcev
Copy link
Owner

Nice! thanks for the PR 🙂

@texastoland texastoland changed the title Bugfix/applicative both Bugfix/applicative both (fixes #3) Sep 11, 2018
@texastoland texastoland deleted the bugfix/applicative-both branch September 11, 2018 04:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants