Make global connection options configurable #77

Merged
merged 2 commits into from Apr 3, 2014

Conversation

Projects
None yet
3 participants
@niels
Contributor

niels commented Jun 18, 2013

An HTTP library agnostic connection_options hash strikes me as a better solution than duplicating all options provided by the HTTP lib within instagram (proxy, ssl, etc.). First, it decouples the instagram from the HTTP library. Second, it makes it easier for users to set more advanced configurations.

If this pull request is well received, I will send a similar one for request options. In particular, it would be useful to be able to set timeouts (which is done on the request level in Faraday).

Note that the first included commit is just some housekeeping that doesn't change any logic.

@yaauie

This comment has been minimized.

Show comment Hide comment
@yaauie

yaauie Jun 26, 2013

Contributor

This change is not backwards-compatible and would require a major-version release; consider making it backwards-compatible and proving it by adding tests.

[edit] I was blind & missed something; see my later comment.

Contributor

yaauie commented Jun 26, 2013

This change is not backwards-compatible and would require a major-version release; consider making it backwards-compatible and proving it by adding tests.

[edit] I was blind & missed something; see my later comment.

@niels

This comment has been minimized.

Show comment Hide comment
@niels

niels Jun 26, 2013

Contributor

How is this change not backwards-compatible? All previously existing tests continue to pass and the new option has a an empty Hash as the default. Not overwriting it thus does not lead to any side effects in which case everything should work as before.

Contributor

niels commented Jun 26, 2013

How is this change not backwards-compatible? All previously existing tests continue to pass and the new option has a an empty Hash as the default. Not overwriting it thus does not lead to any side effects in which case everything should work as before.

@@ -12,7 +12,7 @@ def connection(raw=false)
:proxy => proxy,
:ssl => {:verify => false},
:url => endpoint,
- }
+ }.merge(connection_options)

This comment has been minimized.

Show comment Hide comment
@yaauie

yaauie Jun 26, 2013

Contributor

Damn. I missed this. Backwards-compatible after all.

😳

@yaauie

yaauie Jun 26, 2013

Contributor

Damn. I missed this. Backwards-compatible after all.

😳

This comment has been minimized.

Show comment Hide comment
@niels

niels Jun 26, 2013

Contributor

Yeah, I figured let's just make this optional for now. The currently explicitly defined options can always be retired in a later version if desired.

@niels

niels Jun 26, 2013

Contributor

Yeah, I figured let's just make this optional for now. The currently explicitly defined options can always be retired in a later version if desired.

@heatonjb heatonjb merged commit 322db6f into facebookarchive:master Apr 3, 2014

erinnorr pushed a commit to erinnorr/instagram-ruby-gem that referenced this pull request Feb 1, 2015

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