feat(Observable): initially create Observable.generate factory #1537

Igorbek · 2016-03-26T02:14:51Z

Description:

The PR adds static Observable.generate/GenerateObservable.create method with implementation aligned with RxJS4.

The signature supports passing parameters and a single options:

  static create<T, S>(
    initialState: S,
    condition: ConditionFunc<S>,
    iterate: IterateFunc<S>,
    resultSelector: ResultFunc<S, T>,
    scheduler?: Scheduler): Observable<T>
  static create<S>(
    initialState: S,
    condition: ConditionFunc<S>,
    iterate: IterateFunc<S>,
    scheduler?: Scheduler): Observable<S>
  static create<T, S>(
    options: GenerateOptions<T, S>): Observable<T>

Related issue:
#1482

This is my first PR here, hi everybody :)

kwonoj · 2016-03-26T02:41:26Z

src/Rx.ts

@@ -20,6 +20,7 @@ import './add/observable/fromArray';
 import './add/observable/fromEvent';
 import './add/observable/fromEventPattern';
 import './add/observable/fromPromise';
+import './add/observable/generate';


if this is going under Rx, it may need to be also under kitchenSink as well. (kitchSink contains all of Rx export)

I didn't know what should be included into kitchenSink. I'll include.

@kwonoj kitchen sink imports all of Rx by default, so if it is part of rx, kitchen sink will see it too.

https://github.com/ReactiveX/rxjs/blob/master/src/Rx.KitchenSink.ts#L2

Oops, I missed that. Sorry for bringing confusion.

benlesh · 2016-03-28T23:14:19Z

@Igorbek this PR needs to be flattened and rebased.

Igorbek · 2016-03-29T17:41:46Z

Sure.

Igorbek · 2016-03-29T17:51:15Z

spec/observables/generate-spec.ts

+    const expected = '(12345|)';
+
+    expectObservable(source).toBe(expected);
+  });*/


to enable this test need to fix #1543 first

I've checked in #1544 prior to this and believe this is now unblocked - would you mind to update this one?

Igorbek · 2016-03-29T17:56:31Z

@Blesh, it's been flattened and rebased.

benlesh · 2016-03-29T18:19:42Z

@Igorbek we might need to hold this PR until @kwonoj's #1545 lands, or otherwise coordinate with @kwonoj. I don't think there will be huge changes to the testing code, but there would be some changes for sure.

@kwonoj, what do you think?

kwonoj · 2016-03-29T18:21:29Z

I'm about to check in #1545 now (locally running tests for verification once again) so it'll be soon - would like to ask same way to hold this bit. Apologize to @Igorbek might need one additional rebase.

Igorbek · 2016-03-29T18:35:26Z

Sure, no problem.

Igorbek · 2016-04-01T15:53:12Z

Ok, now it's been fixed, flattened and rebased again.

staltz · 2016-04-01T16:44:42Z

Sorry for reviewing this late in the process, and thank you for the effort so far.

I'd like to remind about the criteria for merging an operator (this was shown as a PR comment template when a PR is opened), so there are still a few things missing until this operator is "complete", namely:

If possible, write a asDiagram test case too, for PNG diagram generation purposes
The operator must be documented in JSDoc style in the implementation file, including also the PNG marble diagram image
The operator should be listed in doc/operators.md in a category of operators
It should also be inserted in the operator decision tree file doc/decision-tree-widget/tree.yml

If you need a reference in how to do those, I recommend looking at

src/MiscJSDoc.ts
src/observable/EmptyObservable.ts
rxjs/spec/Observable-spec.ts

Line 492 in 7efcdc9

asDiagram('create(obs => { obs.next(1); })')
rxjs/doc/decision-tree-widget/tree.yml

Line 349 in 7efcdc9

- label: using custom logic

Igorbek · 2016-04-02T06:01:07Z

Ok, I've just returned from Build and so now will probably have more time this week to complete this journey.

Igorbek · 2016-04-07T09:12:54Z

doc/operators.md

@@ -120,6 +120,7 @@ There are operators for different purposes, and they may be categorized as: crea
 - [`fromEvent`](../class/es6/Observable.js~Observable.html#static-method-fromEvent)
 - [`fromEventPattern`](../class/es6/Observable.js~Observable.html#static-method-fromEventPattern)
 - [`fromPromise`](../class/es6/Observable.js~Observable.html#static-method-fromPromise)
+- `generate`


@staltz should I make it a link by following the pattern around?

Yes, a link is nice 👍

Igorbek · 2016-04-07T09:29:11Z

Ok, looks like I'm done. That's what was changed:

added an overload that accepts an 'options' object without resultSelector but still properly typed with state as a result type
all calls to user functions (iterate, condition and resultSelector) are now in try-catch errors are emitted to subscriber's error channel
reduced unnecessary calls to condition function
added more tests to cover scenarios with/without scheduler specified
added JSDoc to all exported functions and option fields
the operator added to the list in doc and to decision tree

@staltz please review and see my question about doc link

staltz · 2016-04-07T10:23:15Z

spec/observables/generate-spec.ts

    const source = Observable.generate(1, x => x == 1, x => x + 1);
    const expected = '(1|)';

    expectObservable(source).toBe(expected, { '1': 1 });
  });
-  it('should produce all values synchronously', () => {
+
+  asDiagram('generate(1, x => x < 3, x => x + 1)')


Usually we have only one of these asDiagram tests, per file. This third one is the most interesting for a diagram. Can you make the 1st and 2nd test revert back to normal it() test?

Igorbek · 2016-04-11T23:19:16Z

@staltz Please go ahead with your separate PR for the docs. I've slightly changed the docs, so I hope it'll help you. The informal part and long technical description are still missing. I appreciate your assistance.

staltz · 2016-04-12T07:36:11Z

Nice, thanks

benlesh · 2016-04-12T19:45:08Z

@staltz ... this looks okay from my review, but I'm leaving it up to you since it seems like you're waiting for something docs related.

benlesh · 2016-04-12T20:31:52Z

Okay.. this LGTM. Last thing would be to flatten this into one commit. @Igorbek can you flatten this? The commit message should be something like feat(Observable.generate): add generate static creation method with some description of the work done. This is so it shows up cleanly in the CHANGELOG.md at publish time.

I'm publishing in a few minutes, so this will probably land in next week's release.

- add overloads that accepts distinct options and a single options object - add tests with marble diagrams - add JSDoc

Igorbek · 2016-04-12T23:13:31Z

Done.

staltz · 2016-04-13T07:42:25Z

LGTM, thanks Igor

benlesh · 2016-04-14T18:48:29Z

Merged with c03434c... thanks @Igorbek

Igorbek · 2016-04-14T21:19:46Z

@Blesh just curious, why don't you use normal merge and use this strange cherry picking?

staltz · 2016-04-18T07:31:53Z

@Igorbek I think it's because of https://github.com/ReactiveX/rxjs/blob/master/doc/maintainer-guidelines.md

lock · 2018-06-07T04:41:45Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

kwonoj reviewed Mar 26, 2016
View reviewed changes

benlesh added the PR: needs rebase label Mar 28, 2016

Igorbek force-pushed the feature/observable-generate branch from cfeafea to 32b7cf8 Compare March 29, 2016 17:48

Igorbek reviewed Mar 29, 2016
View reviewed changes

benlesh removed the PR: needs rebase label Mar 29, 2016

Igorbek force-pushed the feature/observable-generate branch 2 times, most recently from cce6b1f to 894c721 Compare April 1, 2016 15:47

Igorbek reviewed Apr 7, 2016
View reviewed changes

staltz reviewed Apr 7, 2016
View reviewed changes

feat(Observable.generate): add generate static creation method

e4c9f56

- add overloads that accepts distinct options and a single options object - add tests with marble diagrams - add JSDoc

Igorbek force-pushed the feature/observable-generate branch from 2fe2115 to e4c9f56 Compare April 12, 2016 23:01

benlesh added the PR: lgtm label Apr 14, 2016

benlesh closed this Apr 14, 2016

kwonoj mentioned this pull request Apr 24, 2016

Implement static generate creation methods #1482

Closed

lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Observable): initially create Observable.generate factory #1537

feat(Observable): initially create Observable.generate factory #1537

Igorbek commented Mar 26, 2016

kwonoj Mar 26, 2016

Igorbek Mar 26, 2016

david-driscoll Mar 29, 2016

kwonoj Mar 30, 2016

benlesh commented Mar 28, 2016

Igorbek commented Mar 29, 2016

Igorbek Mar 29, 2016

kwonoj Mar 29, 2016

Igorbek commented Mar 29, 2016

benlesh commented Mar 29, 2016

kwonoj commented Mar 29, 2016

Igorbek commented Mar 29, 2016

Igorbek commented Apr 1, 2016

staltz commented Apr 1, 2016

Igorbek commented Apr 2, 2016

Igorbek Apr 7, 2016

staltz Apr 7, 2016

Igorbek Apr 7, 2016

Igorbek commented Apr 7, 2016

staltz Apr 7, 2016

Igorbek commented Apr 11, 2016

staltz commented Apr 12, 2016

benlesh commented Apr 12, 2016

benlesh commented Apr 12, 2016

Igorbek commented Apr 12, 2016

staltz commented Apr 13, 2016

benlesh commented Apr 14, 2016

Igorbek commented Apr 14, 2016

staltz commented Apr 18, 2016

lock bot commented Jun 7, 2018

feat(Observable): initially create Observable.generate factory #1537

feat(Observable): initially create Observable.generate factory #1537

Conversation

Igorbek commented Mar 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlesh commented Mar 28, 2016

Igorbek commented Mar 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Igorbek commented Mar 29, 2016

benlesh commented Mar 29, 2016

kwonoj commented Mar 29, 2016

Igorbek commented Mar 29, 2016

Igorbek commented Apr 1, 2016

staltz commented Apr 1, 2016

Igorbek commented Apr 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Igorbek commented Apr 7, 2016

Choose a reason for hiding this comment

Igorbek commented Apr 11, 2016

staltz commented Apr 12, 2016

benlesh commented Apr 12, 2016

benlesh commented Apr 12, 2016

Igorbek commented Apr 12, 2016

staltz commented Apr 13, 2016

benlesh commented Apr 14, 2016

Igorbek commented Apr 14, 2016

staltz commented Apr 18, 2016

lock bot commented Jun 7, 2018