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

Add default option on head requests #430

Merged

Conversation

@supremebeing7
Copy link
Contributor

commented Aug 22, 2015

This attempts to resolve #345. I'm pretty confident the code works, but I couldn't get the spec figured out. I left my attempt in there, with the expectation line commented out.

Suggestions welcome/encouraged.

@jnunemaker

This comment has been minimized.

Copy link
Owner

commented Aug 24, 2015

I'm not comfortable merging without a passing spec and I don't have time to look into this right now. If someone else could help, that would be great.

@supremebeing7

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2015

Okay, I had some more time and was able to refactor this a bit to make it more easily testable, then add specs.

I went with the "principle of least surprise" approach outlined by xunker, ensuring that head requests maintain the HEAD method across all redirects, unless the user specifies maintain_method_across_redirects: false in the options.

@@ -529,6 +532,12 @@ def options(path, options = {}, &block)

private

def ensure_method_maintained_across_redirects(options)
unless options.has_key? :maintain_method_across_redirects

This comment has been minimized.

Copy link
@jnunemaker

jnunemaker Aug 26, 2015

Owner

This means it will do a GET if it is set to false, is that right?

This comment has been minimized.

Copy link
@supremebeing7

supremebeing7 Aug 26, 2015

Author Contributor

Exactly. I wanted to make sure there was still a way to override the new behavior, in case someone wanted to do that.

jnunemaker pushed a commit that referenced this pull request Aug 27, 2015
…ects

Add default option on head requests
@jnunemaker jnunemaker merged commit 87af97f into jnunemaker:master Aug 27, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xunker

This comment has been minimized.

Copy link

commented Aug 27, 2015

@supremebeing7 Thanks for taking care of this, I'm glad my research was helpful to you. 👍

@supremebeing7

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2015

@xunker Very helpful 🎉

@supremebeing7 supremebeing7 deleted the supremebeing7:maintain_head_across_redirects branch Aug 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.