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

feat(operator): add elementAt #305

Closed
wants to merge 1 commit into from

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Sep 13, 2015

Effort to add operator exists in current RxJS, picking up relatively simple operator as starting point.


module.exports = function (suite) {

var oldElementAtWithImmediateScheduler = RxOld.Observable.range(0, 25, RxOld.Scheduler.immediate).elementAt(5);
Copy link
Member Author

Choose a reason for hiding this comment

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

As same as test case #303, this operator does not accept any schedulers. I do not feel let those kind of tests belong under current micro test category (immediate / current thread) is correct categorization, maybe need new category such as default?

@kwonoj kwonoj force-pushed the feat-operator-elementat branch 2 times, most recently from 1a708a9 to 783b344 Compare September 13, 2015 18:46
var expected = '-----x|';
var defaultValue = '42';

//expectObservable(source.elementAt(3, defaultValue)).toBe(expected, { x: defaultValue });
Copy link
Member Author

Choose a reason for hiding this comment

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

Tried to follow test case description using marble diagrams but having some trouble with specific case. In case of this, actual result is 42|complete() arrives in same frame. Seems this is related with issue #304 - am I understand correctly?

Copy link
Member

Choose a reason for hiding this comment

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

You mean actual result is -----[42,|]? where [...] happens in the same frame

Copy link
Member Author

Choose a reason for hiding this comment

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

@staltz Yes, correct. My expression wasn't clear enough.

Copy link
Member

Choose a reason for hiding this comment

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

When, it seems clear we need the [a,b] syntax in the DSL then. Even Meijer suggested it

@benlesh
Copy link
Member

benlesh commented Sep 13, 2015

I'm ahead of you. It'll be in there tomorrow

@kwonoj
Copy link
Member Author

kwonoj commented Sep 13, 2015

@Blesh Shall I close PR then?

@benlesh
Copy link
Member

benlesh commented Sep 13, 2015

Nah, leave it open and wait for the testing changes to land.

@benlesh
Copy link
Member

benlesh commented Sep 13, 2015

Testing changes are part of #307


it("should return default value if index is out of range", function() {
var source = hot('--a--|', { a: 'a' });
var expected = '-----x|';
Copy link
Member

Choose a reason for hiding this comment

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

Okay, once you rebase, this can now be:

var expected =   '-----(x|)';   

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, just saw changes merged and about to start work on changes. Thanks!

@kwonoj
Copy link
Member Author

kwonoj commented Sep 14, 2015

Updated changes with

  • now test case updated with grouping marble ability
  • add argumentoutofrange error type

@kwonoj kwonoj force-pushed the feat-operator-elementat branch 2 times, most recently from b422c2c to 3658db5 Compare September 15, 2015 05:55

it("should return default value if index is out of range", function() {
var source = hot('--a--|');
var expected = '-----(x|';
Copy link
Member

Choose a reason for hiding this comment

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

This can be closed like -----(x|), it will technically work like you've done it because of the naive implementation, but it's probably better to close the group in case the implementation changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. let me update quickly to close.

@kwonoj
Copy link
Member Author

kwonoj commented Sep 16, 2015

Updated changes with

  • closed parentheses for test marbles
  • init private field in ctor directly

@kwonoj kwonoj force-pushed the feat-operator-elementat branch 2 times, most recently from 496433e to 87a50f7 Compare September 16, 2015 01:00
@kwonoj
Copy link
Member Author

kwonoj commented Sep 16, 2015

please name index and ...

Additional correction I forgot to do is updated.

@benlesh
Copy link
Member

benlesh commented Sep 16, 2015

Awesome... I'll look at this later this evening or in the morning.

- adds ArgumentOutOfRangeError error type
- adds elementAt operator
@benlesh
Copy link
Member

benlesh commented Sep 16, 2015

merged with cd562c4

thanks @kwonoj!

@benlesh benlesh closed this Sep 16, 2015
@kwonoj kwonoj deleted the feat-operator-elementat branch September 16, 2015 17:08
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
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.

3 participants