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

pass raw options to faraday #189

Merged
merged 1 commit into from Sep 15, 2011
Merged

pass raw options to faraday #189

merged 1 commit into from Sep 15, 2011

Conversation

icambron
Copy link
Contributor

I did this to work around some specific HTTP problem I was having, but it seems like a generally good idea to allow people access to lower-level APIs, just in case there's some Faraday feature they need that isn't in the Twitter gem.

One way to go even further that direction would be to allow the caller to specify their own Faraday middlewares.

sferik added a commit that referenced this pull request Sep 15, 2011
pass raw options to faraday
@sferik sferik merged commit 66c8e19 into sferik:master Sep 15, 2011
@laserlemon
Copy link
Sponsor Collaborator

What, no tests? Also, it might be helpful to do a recursive merge on the faraday options if one wants include a single additional header.

@icambron
Copy link
Contributor Author

There's an implicit test that checks that setting the option works at the Twitter level. As far as testing that the setting really gets set on Faraday, I'm not sure what the best way to do that is. It's also consistent with the other tests. But I'm open to suggestions.

On the recursive merge, yeah, I agree. Another thing that isn't clear is which way the precedence should go. I made it so that Twitter wins when there are conflicting keys, but the more I think about it, the more I think it should go the other way.

@sferik
Copy link
Owner

sferik commented Sep 16, 2011

Should I reopen that issue or are you planning to submit a new pull request to address the recursive merge issue?

@icambron
Copy link
Contributor Author

I think it should be a separate issue, but I'll go ahead and open it, since I don't know when I'll get to it.

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.

None yet

3 participants