feat(core): Add runStream as function from of stream.run #120

briancavalier · 2017-08-31T22:46:37Z

Add runStream as the function form of stream.run.

Open questions

Argument ordering. It's not clear what the "right" argument ordering is. Since the subject of the function is a Stream, the obvious choice is to make it last. After that, though, it's less clear.

Consider that we have two other, similar functions, and it might be nice to have some coherent criteria for ordering the arguments across runStream and these:

runEffects - runEffects :: Stream -> Scheduler, and
runWithLocalTime - runWithLocalTime :: Offset -> Stream a -> Sink a -> Scheduler
- I believe my thinking here was that it made sense for the last two args to mimic the argument ordering of stream.run, i.e. sink, scheduler. Maybe it makes sense to move the Stream to the last arg to end up with Offset -> Sink a -> Scheduler -> Stream a, which is similar to runStream with the extra Offset arg prepended.

Thoughts and suggestions welcome!

Todo

docs

davidchase · 2017-08-31T23:42:45Z

I like stream last approach, in a similar to data last... given you can make a simple sink and have a defaultScheduler but not yet know what the stream is going to be entirely. If that makes any sense 😄

briancavalier · 2017-09-02T18:26:12Z

Ok, I found a coherent argument ordering for runStream and runStreamWithLocalTime (newly renamed from runWithLocalTime) that I think makes sense. I'm not really happy with the name runStreamWithLocalTime, though. It'd be nice to find something shorter and at least as clear.

Ideas?

I'm inclined to keep runEffects' current "flipped" argument order (i.e. scheduler last, Stream a -> Scheduler). I'm having trouble coming up with compelling reasons to justify that other than trying to provide some guidance about calling runEffects once and at the top level, instead of partially applying a Scheduler first and then scattering calls to runEffects around a codebase. Obviously someone can just flip(runEffects), but at least they have to make that conscious decision.

davidchase · 2017-09-03T01:49:37Z

why not just leave it runWithLocalTime and run or runStream ?

briancavalier · 2017-09-03T17:56:02Z

My thinking is that the name run is a little less helpful than runStream, but maybe it's ok, since the types make it obvious.

The "WithLocalTime" suffix, while very explicit, seems a bit wordy. I wonder if there's a way to say the same thing in less than 3 camel humps, like maybe "WithOrigin" or just "At": runWithOrigin, runAt, or runStreamWithOrigin, runStreamAt. Or maybe "Relative": runRelative or runStreamRelative.

Do any of those seem appealing or give you other ideas?

I'm not totally against runWithLocalTime or runStreamWithLocalTime since they will tend to be used less often than runEffects, but if we could find something more concise, that'd be great.

davidchase · 2017-09-03T19:16:04Z

I like run because like you mention types and the fact this is a stream library but maybe that's me with that said I like runWithOrigin or runWithTime if you really want make it obvious .. though for some reason it almost makes sense to make it runWithEffects ?
So something like
run
runWithEffects
runWithOrigin or runWithTime

Thoughts ?

Frikki

Grammar/spelling errors only

Frikki · 2017-09-05T09:27:02Z

packages/core/src/runStream.js

+// Run a stream with its own localized clock
+// This transforms time from the provided scheduler's clock to a stream-local
+// clock (which starts at 0), and then *back* to the scheduler's clock before
+// propagating events to sink.  IOW, stream.run will see local times, and sink


"IOW" is slang for in other words. Google sees it as Isle of Wright. Wikipedia lists:

Isle of Wight (frequently as IoW)

The German abbreviation for the Leibniz Institute for Baltic Sea Research

ISO 639-3 code of the Iowa-Oto language

https://en.wikipedia.org/wiki/IOW

I suggest that you only use official acronyms, so I think you should change it to "In other words, ..."

Thanks! Fixed in 034e260

Frikki · 2017-09-05T09:29:31Z

docs/api.rst

+
+  runStreamWithLocalTime :: Sink a -> Scheduler -> Time -> Stream a -> void
+
+Run a :ref:`Stream`, sending all events to the provided :ref:`Sink`.  The Stream will have localized :ref:`Time` values, whose origin (i.e. time 0) is at the specified Time on the provided :ref:`Scheduler`.


Comma after "i.e.". So:

... whose origin (i.e., time 0)

The same goes for e.g.

Note that the comma after the abbreviations is American style. The British style omits the comma.

Fixed in 034e260

briancavalier · 2017-09-07T11:26:59Z

though for some reason it almost makes sense to make it runWithEffects

I'm not sure I understand. We started with runEffects as the primary application-level API for activating a stream graph, and this PR adds two functions, currently named runStream and runStreamWithLocalTime, primarily to be used when authoring custom stream types. Are you suggesting we rename that to runWithEffects, or that we rename the runStream in this PR to runWithEffects?

One tricky thing now is that all 3 of these functions, runEffects, runStream, and runStreamWithLocalTime, run effects. That is, they all activate a stream and begin consuming all its events and performing any side-effects. They differ in their parameters and return values.

I can even imagine other functions for activating a stream:

-- Provide a handler for error, and a handler for end
cleverName :: (Error -> void) -> (() -> void) -> Stream a -> Scheduler -> void

-- Provide a handler for error only
cleverName2 :: (Error -> void) -> Stream a -> Scheduler -> void

Which maybe opens the can of worms about whether returning a promise is even worthwhile. Given the first variant above, someone could easily do this to get themselves a promise:

new Promise((resolve, reject) => cleverName(resolve, reject, stream, scheduler))

briancavalier · 2017-09-07T11:40:10Z

A few thoughts on short names (even though I don't think short is a necessity here since these functions won't be called all that often):

run
runAt

runStream
runStreamAt

run
runWithTime
(or runWithOrigin)

Frikki · 2017-09-25T05:25:45Z

runLocalized

briancavalier · 2017-09-26T11:09:08Z

See 46a6511, which splits stream clock localization into its own withLocalTime combinator. Instead of needing 3 run-like functions, it means we only need 2: the existing runEffects(), and run(), where run is just the trivial function version of stream.run: run :: Sink a -> Scheduler -> Stream a -> Disposable.

briancavalier · 2017-09-26T11:10:48Z

If we like this direction, then there are a couple remaining todos:

Add unit tests for withLocalTime
Add docs for run and withLocalTime

davidchase · 2017-09-26T11:42:29Z

Love it 😍

briancavalier · 2017-09-27T02:09:58Z

packages/core/src/index.js.flow

+declare export function runStreamWithLocalTime <A> (sink: Sink<A>): (scheduler: Scheduler) => (origin: Time) => (s: Stream<A>) => Disposable
+declare export function runStreamWithLocalTime <A> (sink: Sink<A>, scheduler: Scheduler): (origin: Time, s: Stream<A>) => Disposable
+declare export function runStreamWithLocalTime <A> (sink: Sink<A>, scheduler: Scheduler): (origin: Time) => (s: Stream<A>) => Disposable
+declare export function runStreamWithLocalTime <A> (sink: Sink<A>, scheduler: Scheduler, origin: Time): (s: Stream<A>) => Disposable


Whoops! I need to update the TS and Flow type definitions

briancavalier · 2017-09-27T02:10:18Z

Update TS and Flow type defs

davidchase · 2017-09-27T10:43:08Z

docs/api.rst

+
+  withLocalTime :: Offset -> Stream a -> Stream a
+
+Create a Stream with localized :ref:`Time` values, whose origin (i.e., time 0) is at the specified Time on the :ref:`Scheduler` used to the Stream is observed with :ref:`runEffects` or :ref:`run`.


Scheduler used to the Stream

something maybe missing here ?

Yes. That is not a complete sentence.

In the code comment for withLocalTime, the following text is found:

Create a stream with its own local clock
This transforms time from the provided scheduler's clock to a stream-local clock (which starts at 0), and then back to the scheduler's clock before propagating events to sink. In other words, upstream sources will see local times, and downstream sinks will see non-local (original) times.

I think in this case, the code comment provides some extra detail on how withLocalTime works, which may not be appropriate for user api docs. On the other hand, the last sentence in the comment may be useful in the docs.

From your comment, I'm not sure what you're suggesting. Could you clarify? Thanks!

Frikki · 2017-09-27T17:03:09Z

docs/api.rst

+
+  withLocalTime :: Offset -> Stream a -> Stream a
+
+Create a Stream with localized :ref:`Time` values, whose origin (i.e., time 0) is at the specified Time on the :ref:`Scheduler` used to the Stream is observed with :ref:`runEffects` or :ref:`run`.


Yes. That is not a complete sentence.

Frikki · 2017-09-27T17:10:11Z

docs/api.rst

+
+Create a Stream with localized :ref:`Time` values, whose origin (i.e., time 0) is at the specified Time on the :ref:`Scheduler` used to the Stream is observed with :ref:`runEffects` or :ref:`run`.
+
+When implementing custom higher-order :ref:`Stream` combinators, such as :ref:`chain`, you should use ``withLocalTime`` to localize "inner" Streams before running them.


How is chain a custom combinator that the user implements?

"such as" is referring to chain as a higher-order combinator. Would you suggest removing the word "custom"?

Frikki · 2017-09-27T17:19:13Z

docs/api.rst

+
+  withLocalTime :: Offset -> Stream a -> Stream a
+
+Create a Stream with localized :ref:`Time` values, whose origin (i.e., time 0) is at the specified Time on the :ref:`Scheduler` used to the Stream is observed with :ref:`runEffects` or :ref:`run`.


In the code comment for withLocalTime, the following text is found:

Create a stream with its own local clock
This transforms time from the provided scheduler's clock to a stream-local clock (which starts at 0), and then back to the scheduler's clock before propagating events to sink. In other words, upstream sources will see local times, and downstream sinks will see non-local (original) times.

Frikki · 2017-09-27T17:20:38Z

packages/core/src/combinator/withLocalTime.js

 import { schedulerRelativeTo } from '@most/scheduler'

-// Run a stream with its own localized clock
+// Create a stream with its own local clock


... with its own local clock

Do we also have a clock type? I know there is Time.

dah we do http://mostcore.readthedocs.io/en/latest/api.html#clock

davidchase

💯 row it

briancavalier · 2017-09-29T11:53:39Z

🎉

Frikki requested changes Sep 5, 2017

View reviewed changes

briancavalier force-pushed the add-run-stream-function branch from 21cd8aa to 034e260 Compare September 7, 2017 11:14

briancavalier added 3 commits September 24, 2017 20:42

feat(core): Add runStream as function from of stream.run

652e0df

feat(types): Add curried types for runStream, align naming

12582d3

chore(docs): Fix grammar

c584bea

briancavalier force-pushed the add-run-stream-function branch from 034e260 to c584bea Compare September 25, 2017 00:44

briancavalier self-assigned this Sep 25, 2017

Frikki approved these changes Sep 25, 2017

View reviewed changes

feat(withLocalTime): Factor out local time handling to withLocalTime

46a6511

TylorS approved these changes Sep 26, 2017

View reviewed changes

briancavalier added 4 commits September 26, 2017 21:30

docs(api): Add withLocalTime docs

279ce21

test(withLocalTime): Add withLocalTime test

a7e6d42

docs(api): Add chain ref

d8fc9fd

chore(timestamp): remove unneeded constructor

84f33e1

briancavalier commented Sep 27, 2017

View reviewed changes

davidchase requested changes Sep 27, 2017

View reviewed changes

Frikki requested changes Sep 27, 2017

View reviewed changes

Update types

68171fe

Frikki approved these changes Sep 29, 2017

View reviewed changes

davidchase approved these changes Sep 29, 2017

View reviewed changes

briancavalier merged commit 0f6d27b into master Sep 29, 2017

briancavalier deleted the add-run-stream-function branch September 29, 2017 11:53

TylorS added the 🛠️ Type: Feature label Oct 26, 2017


		runStreamWithLocalTime :: Sink a -> Scheduler -> Time -> Stream a -> void

		Run a :ref:`Stream`, sending all events to the provided :ref:`Sink`. The Stream will have localized :ref:`Time` values, whose origin (i.e. time 0) is at the specified Time on the provided :ref:`Scheduler`.


		withLocalTime :: Offset -> Stream a -> Stream a

		Create a Stream with localized :ref:`Time` values, whose origin (i.e., time 0) is at the specified Time on the :ref:`Scheduler` used to the Stream is observed with :ref:`runEffects` or :ref:`run`.


		Create a Stream with localized :ref:`Time` values, whose origin (i.e., time 0) is at the specified Time on the :ref:`Scheduler` used to the Stream is observed with :ref:`runEffects` or :ref:`run`.

		When implementing custom higher-order :ref:`Stream` combinators, such as :ref:`chain`, you should use ``withLocalTime`` to localize "inner" Streams before running them.

feat(core): Add runStream as function from of stream.run #120

feat(core): Add runStream as function from of stream.run #120

Uh oh!

Conversation

briancavalier commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Open questions

Todo

Uh oh!

davidchase commented Aug 31, 2017

Uh oh!

briancavalier commented Sep 2, 2017

Uh oh!

davidchase commented Sep 3, 2017

Uh oh!

briancavalier commented Sep 3, 2017

Uh oh!

davidchase commented Sep 3, 2017

Uh oh!

Frikki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briancavalier commented Sep 7, 2017

Uh oh!

briancavalier commented Sep 7, 2017

Uh oh!

Frikki commented Sep 25, 2017

Uh oh!

briancavalier commented Sep 26, 2017

Uh oh!

briancavalier commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidchase commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briancavalier commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidchase left a comment

Choose a reason for hiding this comment

Uh oh!

briancavalier commented Sep 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

briancavalier commented Aug 31, 2017 •

edited

Loading

briancavalier commented Sep 26, 2017 •

edited

Loading

briancavalier commented Sep 27, 2017 •

edited

Loading