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 faraday connection options without breaking api #80

Merged
merged 1 commit into from
Dec 2, 2014
Merged

add faraday connection options without breaking api #80

merged 1 commit into from
Dec 2, 2014

Conversation

koenpunt
Copy link
Contributor

SSL options can only be passed to Faraday on initialization. So I've added a connection_options property which is passed to Faraday directly.

Hyperclient.new('https://api.example.com/') do |client|
  client.connection_options = { ssl: { verify: false } }
  client.connection do |conn|
    conn.authorization :Bearer, 'ioP_pcKLhB4NLMGKMM6B40rwkBC.5o2JpsnBHhmKs4QAExG'
  end
end

I think it would be better if the options hash of connection would be passed to Faraday, then we can simply write:

Hyperclient.new('https://api.example.com/') do |client|
  client.connection(ssl: { verify: false }) do |conn|
    conn.authorization :Bearer, 'ioP_pcKLhB4NLMGKMM6B40rwkBC.5o2JpsnBHhmKs4QAExG'
  end
end

But this means the use options[:default] should be changed to something like a property of the client, and thus breaking the API.

@koenpunt
Copy link
Contributor Author

Just found that I can use the update method on the SslOptions:

Hyperclient.new('https://api.example.com/') do |client|
  client.connection do |conn|
    conn.ssl.update(verify: false)
    conn.authorization :Bearer, 'ioP_pcKLhB4NLMGKMM6B40rwkBC.5o2JpsnBHhmKs4QAExG'
  end
end

But I still think it would be nice to set the Faraday options in the connection method.

@dblock
Copy link
Collaborator

dblock commented Nov 30, 2014

I'm down with this, but it needs a test, README and CHANGELOG updates, please.

@koenpunt
Copy link
Contributor Author

koenpunt commented Dec 2, 2014

:default is now implicit. Which means that default: true is not necessary, but also means this change is backwards compatible.

if block_given?
fail ConnectionAlreadyInitializedError if @connection
if options[:default]
if @faraday_options.delete(:default) == false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should be deleteing items from here because this is a hash passed from the outside. Also comparing to false is probably a bad idea. If you need to check whether the value is present at all, use key?.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because :default is of no use for Faraday. But yeah, I'm not exactly cool with it either..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use @faraday_options = options.dup though

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are going to delete, yes.

@dblock
Copy link
Collaborator

dblock commented Dec 2, 2014

This is nice work. See my comment(s) above. Don't forget CHANGELOG, README and please squash the commits if appropriate.

@koenpunt
Copy link
Contributor Author

koenpunt commented Dec 2, 2014

Working on the README and CHANGELOG as we speak ;)

@koenpunt
Copy link
Contributor Author

koenpunt commented Dec 2, 2014

The weird whitespace in entry_point_test.rb is because not all describe blocks were nested in describe EntryPoint do

default is implicit, non default explicit
update rubocop dependency
update README and CHANGELOG
@@ -2,10 +2,11 @@

* Your contribution here.

### 0.7.0-pre (December 1, 2014)
### 0.7.0 (December 2, 2014)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I usually just keep these things under Next. That's because there's no actual -pre release, so it may be confusing for someone reading it. It's no big deal.

@dblock
Copy link
Collaborator

dblock commented Dec 2, 2014

Merging.

dblock added a commit that referenced this pull request Dec 2, 2014
add faraday connection options without breaking api
@dblock dblock merged commit 9f90885 into codegram:master Dec 2, 2014
@koenpunt koenpunt deleted the faraday-options branch December 2, 2014 14:29
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.

2 participants