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

Reset default request headers #508

Closed

Conversation

tomatoturnip
Copy link

Made a fix to reset default request headers re: #506

@CJStadler
Copy link

@tloggins and I ran into this issue and I've verified that this PR fixes it for us. Thanks @tomatoturnip!

@CJStadler
Copy link

@jnunemaker any chance you could review this? It would be much appreciated 🙇

@jnunemaker
Copy link
Owner

Extremely sorry I overlooked this. Not sure what went wrong.

So the goal of this is to not pass headers to net/http if none are specified in httparty so that net/http's default headers are used?

@CJStadler
Copy link

@jnunemaker that is correct.

Instead of doing this cleanup before every request perhaps it would be cleaner to either

  1. Change process_headers to use default_options[:headers] instead of headers.
  2. Change headers to not modify default_options[:headers] when it does not receive an arg.

@CJStadler
Copy link

CJStadler commented Apr 10, 2017

@jnunemaker sorry to ping you on this again, but it caused another unexpected build failure today. I'm happy to open a PR with either of the above two solutions, if you'd prefer that to the current solution. Thanks!

@jnunemaker
Copy link
Owner

@CJStadler no prob! I would be fine with 1 or 2. Which feels better to you?

@jnunemaker
Copy link
Owner

@tomatoturnip I'm going to close this one. Again, very sorry I overlooked it. Once I did look it over, I didn't quite understand the problem. Now that I do, I feel that @CJStadler suggestions would be better. My main problem with this one is that it deletes things, which feels a little brute force. I'd rather see some logic that avoids the delete. I'd be happy to merge this if you want to update it or I can wait for @CJStadler's PR.

@jnunemaker jnunemaker closed this Apr 11, 2017
@tomatoturnip
Copy link
Author

@jnunemaker No worries. @CJStadler can create the PR. His suggestions sound good. Thanks guys! :)

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.

3 participants