Skip to content

Pass options to dynamic block headers#661

Merged
TheSmartnik merged 1 commit intojnunemaker:masterfrom
wojw5:pass-options-to-block-headers
Aug 30, 2019
Merged

Pass options to dynamic block headers#661
TheSmartnik merged 1 commit intojnunemaker:masterfrom
wojw5:pass-options-to-block-headers

Conversation

@wojw5
Copy link
Contributor

@wojw5 wojw5 commented Jul 9, 2019

I encountered the following problem: API required checksum based on the request body.

This simple change allows using request options in block header like in this example:

headers 'X-SIGNATURE' => lambda { |options|
          Digest::SHA256.hexdigest(options[:body] + configuration.api_secret)
        }

lib/httparty.rb Outdated

def process_dynamic_headers(headers)
headers.each_with_object({}) do |header, processed_headers|
def process_dynamic_headers(options)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it would be better to move it to a separate class? Something like HeaderProcessor. I don't really like that we do everything in a base class. Additionally, the method became more complex, so it's justified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TheSmartnik please preview code with extracted HeadersProcessor class

@wojw5 wojw5 force-pushed the pass-options-to-block-headers branch 3 times, most recently from 508f504 to b9df779 Compare August 21, 2019 10:20
@wojw5 wojw5 force-pushed the pass-options-to-block-headers branch from b9df779 to 1d8d0a7 Compare August 21, 2019 10:23
private

def process_dynamic_headers
options[:headers].each_with_object({}) do |header, processed_headers|
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just, fyi. You can do |(key, value), processed_headers|

@TheSmartnik TheSmartnik merged commit d2ff83b into jnunemaker:master Aug 30, 2019
@TheSmartnik
Copy link
Collaborator

@wojw5 thanks! I like what you did here, looks nice

@wojw5
Copy link
Contributor Author

wojw5 commented Sep 5, 2019

@TheSmartnik Do you think this is enough to bump version and do gem release? I have another gem that depends on this feature and i will release it to RubyGems once this dependency will be on RubyGems as well.

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