Skip to content

Conversation

@seanmonstar
Copy link
Member

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.15%) to 85.45% when pulling 5176922 on drop-res into 7f5e038 on master.

@Ryman
Copy link
Contributor

Ryman commented May 6, 2015

The drop tests don't seem to actually test the body itself is flushed?

The branching on PhantomData looks like a neat technique!

Copy link
Contributor

Choose a reason for hiding this comment

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

This is inspired.

Copy link
Contributor

Choose a reason for hiding this comment

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

That said we can get better perf by just doing TypeId::of::<W>() == TypeId::of::<StatusFresh>(), which will get the branch optimized away because monomorphization.

@seanmonstar
Copy link
Member Author

@Ryman what do you mean? That flush was called on the MockStream?

Copy link
Contributor

Choose a reason for hiding this comment

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

should lose the _ now that it is used

EDIT: nvm because the suggestion in the other comment makes it unused again

@Ryman
Copy link
Contributor

Ryman commented May 6, 2015

@seanmonstar I mean that the tests seem to only check with a zero byte body in the response, I wonder if it's a benefit to include a test that actually ensures a body is dealt with appropriately?

@seanmonstar
Copy link
Member Author

@Ryman ah sure. Though, since it's a chunked the response, that last chunk written before flushing must be the 0\r\n\r\n chunk, which the tests are checking for.

seanmonstar added a commit that referenced this pull request May 6, 2015
feat(server): dropping a Response will write out to the underlying stream
@seanmonstar seanmonstar merged commit d294ea5 into master May 6, 2015
@seanmonstar seanmonstar deleted the drop-res branch May 6, 2015 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants