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

bufferToggle: fixes and tests #538

Closed
wants to merge 2 commits into from

Conversation

staltz
Copy link
Member

@staltz staltz commented Oct 15, 2015

No description provided.

Andre Medeiros added 2 commits October 15, 2015 14:43
@benlesh
Copy link
Member

benlesh commented Oct 15, 2015

merged with 949fa31 ... thanks @staltz ..

Looking at that bit with context, I wonder if it would have been better to have a sub class of Subscription that carried the buffer with it and managed semantics around buffer size and completion... Dunno. I think your solution is fine.

@benlesh benlesh closed this Oct 15, 2015
@staltz staltz deleted the tests-bufferToggle branch October 16, 2015 09:49
@staltz
Copy link
Member Author

staltz commented Oct 16, 2015

I wonder if it would have been better to have a sub class of Subscription that carried the buffer with it

That didn't occur to me when I was fixing, you mean it would be better in performance or in elegance? I just tried to do minimal changes in order to get tests passing.

@benlesh
Copy link
Member

benlesh commented Oct 16, 2015

I'd be curious about either, honestly. But it's not a big deal

@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.

None yet

2 participants