Added destroy method for stream #12

Merged
merged 1 commit into from Jul 16, 2012

Conversation

Projects
None yet
2 participants
Contributor

koenpunt commented Jul 16, 2012

Implemented missing method destroy on stream and added simple test for streaming API

Added destroy method for stream
Simple test for streaming API
Replaced hard-tabs with with soft-tabs

@koenpunt koenpunt closed this Jul 16, 2012

impronunciable added a commit that referenced this pull request Jul 16, 2012

@impronunciable impronunciable merged commit 75a6477 into impronunciable:master Jul 16, 2012

Contributor

koenpunt commented Jul 16, 2012

Ok, but maybe initiating a Stream object is better than creating a custom destroy... Trying that right now

Owner

impronunciable commented Jul 16, 2012

I thought about that. First I merged this to have things working and then need to implement the stream as a real stream...

Contributor

koenpunt commented Jul 16, 2012

I guess the custom Stream object isn't even necessary, because req is already an instance of Stream.

Owner

impronunciable commented Jul 16, 2012

I don't want to return back to the user the whole request to handle. I like this approach with a custom stream object containing only the methods and data that is relevant to the user.

Contributor

koenpunt commented Jul 16, 2012

Ok fair enough, maybe initiating Stream like:

var stream = new Stream(req);

and setting the request in Stream as instance variable

function Stream(req){
    this.request = req;
}
Stream.prototype.destroy = function(){
    this.request.destroy();
}

so the original request is also available?

I've submitted a PR for this: #13

Owner

impronunciable commented Jul 16, 2012

There is no need to do that. The only thing I want to implement is a function that destroy the stream, so I can start a new one for a possible reconnection.

Also if I'm starting using a stream from the Stream module I need to use another name to the 'error' event, that means I need to change the API of the module and I don't want that.

I think that adding the new destroy function it's good, at least for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment