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

stream_body fragment include http_response feature #588

Merged

Conversation

shanempope
Copy link
Contributor

Add a :stream_body_include_response_code option to get a response code back with the fragment when streaming data. This allows for skipping redirects while streaming (and other error detection), which before would tack the redirect onto the file.

We needed this, I appreciate any suggestions.

@shanempope shanempope changed the title stream_body_include_response_code feature [DNM-WIP]stream_body_include_response_code feature Apr 24, 2018
@shanempope shanempope changed the title [DNM-WIP]stream_body_include_response_code feature stream_body_include_response_code feature Apr 24, 2018
@shanempope
Copy link
Contributor Author

shanempope commented Apr 24, 2018

How we use this:

  url = 'SOMETHING THAT REDIRECTS using 302'
  HTTParty.get(url, stream_body: true, stream_body_include_response_code: true) do |resp_fragment|
    file.write(resp_fragment[:fragment]) if resp_fragment[:code] == 200
  end

Copy link
Owner

@jnunemaker jnunemaker left a comment

Choose a reason for hiding this comment

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

@shanempope thanks so much for taking the time to create a pull request, include specs and show your example usage. That is very helpful.

My concern with this PR is the lack of symmetry in what gets yielded (without new option it yields chunk, with new option it yields hash). I think I'd like to see the same thing get yielded either way and avoid an option.

I wonder if we did some sort of backwards compatible decorator if that would be enough:

# new class that allows access to http response and delegates everything else to fragment
class FragmentWithResponse < SimpleDelegator
  extend Forwardable
  
  def_delegator :@http_response, :code

  attr_reader :http_response

  def initialize(fragment, http_response)
    @fragment = fragment
    @http_response = http_response
    super fragment
  end
end

http_response.read_body do |fragment|
  chunks << fragment unless options[:stream_body]
  block.call FragmentWithResponse.new(fragment, http_response)
end

That would allow you to do:

url = 'SOMETHING THAT REDIRECTS using 302'
HTTParty.get(url, stream_body: true) do |fragment|
  file.write(fragment) if fragment.code == 200
end

My only concern would be hoping/assuming that file.write and all other uses of a fragment currently use duck typing instead of type checking. I wouldn't want people to start writing inspect fragment decorator objects instead of strings accidentally.

Any thoughts on any of the above?

@shanempope
Copy link
Contributor Author

I seriously appreciate you taking the time to look at my pull request. I quite like your approach. I am too inexperienced in ruby to understand the ramifications of making it a forwardable, and any breaking effects it could have on your users. I assume, if it's possible to do something a certain way in ruby, someone, somewhere is using your library that way.

I made the changes and pushed them. Let me know what you think.
-shane

@shanempope shanempope force-pushed the stream_body_include_response_code branch from 1338992 to c3c781e Compare April 25, 2018 21:58
@shanempope shanempope changed the title stream_body_include_response_code feature stream_body fragment include http_response feature Apr 26, 2018
@shanempope
Copy link
Contributor Author

@jnunemaker Do you think this will ever make it in? Thanks

@TheSmartnik
Copy link
Collaborator

@shanempope sorry for the delay. I'll look into it later today

@TheSmartnik
Copy link
Collaborator

@shanempope everything seems fine, thanks for the pr. Can you please add example usage to examples directory so this feature wouldn't be lost

I also switched FragmentWithResponse.code to return an int which matches how the normal HTTParty response treats them.
@shanempope
Copy link
Contributor Author

@TheSmartnik Thanks, I added an example and fixed FragmentWithResponse.code to be an int to match the way HTTParty's response.code is.

@TheSmartnik
Copy link
Collaborator

@shanempope thanks! Looks good

@TheSmartnik TheSmartnik merged commit b842040 into jnunemaker:master Jan 23, 2019
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.

3 participants