Skip to content

fix(windowCount): set default value for skip argument, do not emit em…#273

Closed
kwonoj wants to merge 1 commit intoReactiveX:masterfrom
kwonoj:fix-operator-windowcount
Closed

fix(windowCount): set default value for skip argument, do not emit em…#273
kwonoj wants to merge 1 commit intoReactiveX:masterfrom
kwonoj:fix-operator-windowcount

Conversation

@kwonoj
Copy link
Member

@kwonoj kwonoj commented Sep 6, 2015

…pty buffer at the end.

As same as bufferCount implementation fixed in #271, windowCount also does not sets default skip argument if it's not specified. This PR windowCount to set default skip argument to size of window.

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 mentioned in #271 (comment), some function initialize default value to 0 and some sets it to null for number types. As same as previous changes only updates value but not default value since not sure which one's recommended.

@benlesh
Copy link
Member

benlesh commented Sep 8, 2015

Thanks for catching these, @kwonoj

merged as of a513dbb

@benlesh benlesh closed this Sep 8, 2015
@kwonoj kwonoj deleted the fix-operator-windowcount branch September 8, 2015 16:46
staltz pushed a commit to staltz/RxJSNext that referenced this pull request Oct 13, 2015
Fix bugs with windowCount. This commit reverts PR ReactiveX#273, in order to have
windowCount pass comprehensive marble tests. windowCount must open the
first window immediately, not when the first next() event arrives, to
comply with legacy windowWithCount and with RxJS Next window and
windowTime.
benlesh pushed a commit that referenced this pull request Oct 13, 2015
Fix bugs with windowCount. This commit reverts PR #273, in order to have
windowCount pass comprehensive marble tests. windowCount must open the
first window immediately, not when the first next() event arrives, to
comply with legacy windowWithCount and with RxJS Next window and
windowTime.
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 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