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

ensuring that GET and HEAD returns the same headers. (#354) #357

Merged
merged 2 commits into from Apr 3, 2015

Conversation

Projects
None yet
3 participants
@kazu-yamamoto
Contributor

kazu-yamamoto commented Apr 2, 2015

@DeathByTape @snoyberg Please review this patch.

@snoyberg

This comment has been minimized.

Member

snoyberg commented Apr 2, 2015

@DeathByTape I think we need to modify the autohead middleware to keep the ResponseFile response if available.

@kazu-yamamoto I'm actually somewhat confused about the isGet stuff; isn't the point to check if it's not HEAD?

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Apr 2, 2015

isGet means isNotHead in this case. Other methods are unsuitable here, I believe.

@snoyberg

This comment has been minimized.

Member

snoyberg commented Apr 2, 2015

I may be reading this code incorrectly, but I thought the code path is simply for a response which is returning a ResponseFile. While a GET response is by far the most likely case for that, it seems reasonable that an application could similarly use a ResponseFile for a POST or PUT request if desired. Is there any downside to changing isGet to isNotHead?

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Apr 2, 2015

I wanted to give a name of hasBody to this variable. But I did not since it is used already.

I would avoid negative names for readability. Any suggestions? What about useSendFile?

@snoyberg

This comment has been minimized.

Member

snoyberg commented Apr 2, 2015

How about isHead and make the test not isHead?

@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Apr 2, 2015

Is ed5afae what you want?

@DennisMcWherter

This comment has been minimized.

Contributor

DennisMcWherter commented Apr 2, 2015

I submitted #358 with some changes to the middleware (effectively neutralizes it) and update wai-app-static to handle this. It is a PR into this branch (get-head) since it all seems like a single feature to me. I can change the PR location to master, however, if that's better.

@snoyberg

This comment has been minimized.

Member

snoyberg commented on ed5afae Apr 2, 2015

Yes, I think this commit gives the code the right semantics. Thanks!

Merge pull request #358 from DeathByTape/get-head
Update wai-app-static for proper HEAD support
@kazu-yamamoto

This comment has been minimized.

Contributor

kazu-yamamoto commented Apr 3, 2015

@snoyberg Please merge this pull request to master if you think OK.

@snoyberg snoyberg merged commit 0838caa into master Apr 3, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@snoyberg snoyberg deleted the get-head branch Apr 3, 2015

@snoyberg

This comment has been minimized.

Member

snoyberg commented Apr 3, 2015

Merged and released, thanks guys!

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