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.

Sign up for GitHub

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

Jump to bottom

feat(windowTime): maxWindowSize parameter in windowTime operator #2187

Merged
merged 1 commit into from
Feb 15, 2017
Merged

feat(windowTime): maxWindowSize parameter in windowTime operator #2187

merged 1 commit into from
Feb 15, 2017

Conversation

mpodlasin
Copy link
Contributor

Description:

Adds new parameter in windowTime operator to control how much values given
window can emit.

Related issue (if exists):

Closes #1301

@coveralls
Copy link

coveralls commented Dec 12, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.009%) to 97.696% when pulling bb52bbe on Podlas29:window-time-with-max-size into 89b506d on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Dec 12, 2016

This LGTM... @kwonoj can I get a second review here?

@coveralls
Copy link

coveralls commented Dec 12, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.009%) to 97.683% when pulling 1c9f746 on Podlas29:window-time-with-max-size into b5a3413 on ReactiveX:master.

kwonoj
kwonoj approved these changes Dec 12, 2016
View reviewed changes
src/operator/windowTime.ts Outdated
this.numberOfNextedValues++;
}

getNumberOfNextedValues() {
Copy link
Member

Choose a reason for hiding this comment

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

is this need to be function? seems

public get nextedValueLength() {
 return this.numberOfNextedValues;
}

would work. Nitpicking though.

jayphelps
jayphelps reviewed Dec 12, 2016
View reviewed changes
src/operator/windowTime.ts Outdated
@@ -49,6 +60,8 @@ import { Subscription } from '../Subscription';
* @param {number} windowTimeSpan The amount of time to fill each window.
* @param {number} [windowCreationInterval] The interval at which to start new
* windows.
* @param {number} [maxWindowSize=Numbe.POSITIVE_INFINITY] Max number of
Copy link
Member

Choose a reason for hiding this comment

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

typo s/Numbe.POSITIVE_INFINITY/Number.POSITIVE_INFINITY

@kwonoj
Copy link
Member

kwonoj commented Dec 12, 2016

LGTM, minor nitpicking suggestions but it should not block check in PR and can be updated later.

jayphelps
jayphelps reviewed Dec 12, 2016
View reviewed changes
src/operator/windowTime.ts Outdated

next(value?: T) {
super.next(value);
this.numberOfNextedValues++;
Copy link
Member

Choose a reason for hiding this comment

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

This increment needs to happen before we super.next(value) otherwise someone can recursively reenter and never reach the max count because it hasn't been incremented yet.

@jayphelps
Copy link
Member

@Blesh and I were talking about this a bit and want to hold off merging, but may merge when we have time to flush our concerns that were discovered. Sit tight. 👍

@benlesh
Copy link
Member

benlesh commented Dec 12, 2016

I think we're going to leave this one for the next minor version. There are a few issues I can see around reentracy (that @jayphelps pointed out), and some tweaks I'd like to make.

@mpodlasin
Copy link
Contributor Author

Any way I can help with this one some more, or should I just leave it for now?

@jayphelps
Copy link
Member

@Podlas29 leave for now, we'll elaborate soon. 🤘

@jayphelps
Copy link
Member

jayphelps commented Jan 31, 2017 •
edited
Loading

@Podlas29 we discussed this at our core team meeting today. We dig it, but it has that reentry problem that I commented on: #2187 (comment)

It unfortunately is also now out of date due to some recent changes in #2277 and #2278. Resolving the conflicts might be a bit involved, cause we don't want to accidentally lose those other changes too. Could you give it a try for us and fix the reentry problem by moving the this. numberOfNextedValues++ ?

@mpodlasin
Copy link
Contributor Author

@jayphelps No problem, I'm on it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 97.711% when pulling 4c5ab7e on Podlas29:window-time-with-max-size into a77821b on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 97.711% when pulling 2549e88 on Podlas29:window-time-with-max-size into a77821b on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 97.711% when pulling 2cbdc30 on Podlas29:window-time-with-max-size into a77821b on ReactiveX:master.

@mpodlasin
Copy link
Contributor Author

@jayphelps ok should be fine now. I made requested changes to order of operations, fixed typo and rebased to current master as carefully as possible.

jayphelps
jayphelps approved these changes Jan 31, 2017
View reviewed changes
Copy link
Member

@jayphelps jayphelps left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, Can I get one other review from someone please? Perhaps @kwonoj since he touched this code most recently. (I see you reviewed it earlier, but he had to resolve conflicts on rebase against your changes so want your eyes)

kwonoj
kwonoj approved these changes Feb 13, 2017
View reviewed changes
src/operator/windowTime.ts Outdated
private numberOfNextedValues: number;
constructor() {
super();
this.numberOfNextedValues = 0;
Copy link
Member

Choose a reason for hiding this comment

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

you can make 148 private numberof.. = 0 and remove ctor.

src/operator/windowTime.ts Outdated
super.next(value);
}

getNumberOfNextedValues(): number {
Copy link
Member

Choose a reason for hiding this comment

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

for this one we can simply make it as getter property?

private _numberOfNextedValues: number = 0;
public get numberOfNextedValues() {
 return this._numberOfNextedValues;
}

This also fits for our further directions to follow private property convention (_xxx) for JS users.

@kwonoj
Copy link
Member

kwonoj commented Feb 13, 2017

Minor nitpicking, and needs rebase - I know it's not conflicting but not reflect recent type inference in test, enabling it will check new tests conforms type inferences as well.

Once it's rebased I'll check & merge.

@kwonoj kwonoj self-assigned this Feb 13, 2017
@jayphelps
Copy link
Member

@kwonoj rebased against upstream/master

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 97.686% when pulling f6bb310 on Podlas29:window-time-with-max-size into de6a8f4 on ReactiveX:master.

@mpodlasin
feat(windowTime): maxWindowSize parameter in windowTime operator
381be3f 
Adds new parameter in windowTime operator to control how much values given
window can emit.

Closes #1301
@coveralls
Copy link

coveralls commented Feb 15, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.009%) to 97.711% when pulling 2549e88 on Podlas29:window-time-with-max-size into a77821b on ReactiveX:master.

@mpodlasin
Copy link
Contributor Author

@kwonoj I made the changes you requested.

@jayphelps jayphelps merged commit db8dc77 into ReactiveX:master Feb 15, 2017
@lock
Copy link

lock bot commented Jun 6, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kwonoj kwonoj kwonoj approved these changes

@jayphelps jayphelps jayphelps approved these changes

Assignees

@kwonoj kwonoj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mpodlasin @coveralls @benlesh @kwonoj @jayphelps