-
Notifications
You must be signed in to change notification settings - Fork 173
Add support for setting default headers on http-opts #298
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
Conversation
|
@zentourist Hey Dan, any thoughts on this change? |
Hey @MhRantz - I'm out of the office until next week. Let me have a look when I'm back. ✌️ |
zentourist
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logically it looks good, but I think we can clean it up a bit and allow for easier future extensions.
I no longer have access to Exchange and don't write much Ruby these days, so you'll want to give it a whirl, but this should work.
| # @option opts [String] :user_agent the http user agent to use in all requests | ||
| def initialize(endpoint, opts = {}) | ||
| @log = Logging.logger[self.class.name.to_s.to_sym] | ||
| if opts[:user_agent] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to properly support both options, you'll want to filter the supported opts and pass them all in at once. As it is written, if you send in a :user_agent opt and a :default_header opt, only the :default_header will be applied. I would suggest something like the following:
Add a class var to the top of the Connection class:
@@supported_httpclient_opts = %i[agent_name default_header]Then remove the if/elseif/else statement in favor of this:
httpclient_opts = opts.slice(@@supported_httpclient_opts)
@httpcli = HTTPClient.new(**httpclient_opts)
zentourist
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @MhRantz!
|
@MhRantz - I published this gem as 1.2.0 because there was a number of other commits that had not been released yet. |
Allow for setting default headers beyond agent_name adding customization
Note: Looks like changelog.txt is not is use, but happy to update.