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

Extend the Reflex class with now :: PushM t (Event t ()). (Issue #414) #416

Merged
merged 14 commits into from
May 14, 2020

Conversation

parenthetical
Copy link
Contributor

I hope the implementation in Spider is correct, it produced the expected result when testing with switchHoldPromptly never . pushAlways (\a -> fmap (a <$) now).

Extending the Reflex class might break modules depending on Reflex if they or another dependency use now as an identifier. Let me know if that's not okay.

The change is motivated by distributed Reflex (coming soon™), in which
you can write things like:

roundTrip :: (...) => Location t -> m (Event t ())
roundTrip to = headE =<< perceptionsHere =<< perceptionsAt to =<< now

Maybe there are other use cases as well?

@parenthetical parenthetical changed the title Extend the Reflex class with now :: PushM t (Event t ()). (#414) Extend the Reflex class with now :: PushM t (Event t ()). (Issue #414) May 1, 2020
@3noch 3noch requested a review from ryantrinkle May 1, 2020 16:00
@ryantrinkle
Copy link
Member

Overall this looks good. Would you mind adding a new EventSubscribed, instead of reusing eventSubscribedNever? They probably ought to be the same, except for, e.g.

, eventSubscribedWhoCreated = return ["never"]

Also, we'll need some tests for this.

@parenthetical
Copy link
Contributor Author

I'll change that and try to add some tests as well, thanks!

@parenthetical
Copy link
Contributor Author

Does this look better?

@ryantrinkle
Copy link
Member

Can we add a test showing that it is not firing at some times other than the time it's created? I think it might need an IORef or something - this looks like it is probably always firing (but only the first time each subscriber looks at it).

@parenthetical
Copy link
Contributor Author

Thanks for the hints; I implemented it with an IORef and added a test case for the always firing issue.
Was scheduleClear was the right procedure to use?

@ryantrinkle
Copy link
Member

Awesome! That looks right to me :)

Only two things left:

  1. You should add something to the changelog announcing this!
  2. We'll take a look at what's going wrong with CI - I think that might be on our end.

@parenthetical
Copy link
Contributor Author

parenthetical commented May 5, 2020

Great, thanks for the shepherding!
However, I just tried writing a program with it and now I'm not sure whether adding now to Reflex was the right thing to do, since I immediately needed a HasNow type class as well (which then has to use a different identifier ideally).
What would you suggest in this case? I could also extend MonadSample or MonadHold instead of Reflex.

@ryantrinkle
Copy link
Member

Hmm, well, putting it in a typeclass seems like a perfectly reasonable thing to do. Note that not all implementations of MonadSample or MonadHold will have implementations for now. In particular: I think it'd be reasonable for people to assume that hold a . (b <$) =<< now will always update the Behavior to b, but that's not possible outside of a frame. And, of course, PullM can't use now because cached results would be wrong.

@@ -420,6 +420,10 @@ class MonadSample t m => MonadHold t m where
-- | Create a new 'Event' that only occurs only once, on the first occurrence of
-- the supplied 'Event'.
headE :: Event t a -> m (Event t a)
-- | An event which occurs at the current moment.
now :: m (Event t ())
Copy link
Member

Choose a reason for hiding this comment

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

Can any laws be documented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this if that's what you're thinking of?

  -- | An event which only occurs at the current moment in time, such that:
  --
  -- > coincidence (pushAlways (\a -> (a <$) <$> now) e) = e
  --
  now :: m (Event t ())

Also, now turns events in the context of a MonadHold into an applicative functor and monad using the existing Apply and Bind instances:

instance (..., MonadHold t m) => Applicative (m . Event t) where
  pure a = (a <$) <$> now
  (<$>) = liftA2 (<.>)

@ryantrinkle ryantrinkle merged commit 17ccde7 into reflex-frp:develop May 14, 2020
@ryantrinkle
Copy link
Member

Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants