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 ability to pass encoder to HTTP::FormData::Urlencoded instance #29

Merged
merged 5 commits into from
Mar 3, 2020

Conversation

summera
Copy link
Contributor

@summera summera commented Mar 3, 2020

Since encoder is global and currently defined at the class level, it's difficult to customize on a per user-case basis. For example, there's an issue with the Twitter gem (sferik/twitter-ruby#677), which depends on this gem, where the default encoding of asterisks does not work and Twitter will throw an error. The only way around this is to customize the encoding, but being forced to customize encoding at the class level makes things difficult since other gems could be using http-form_data. There's also no easy way to figure out if the Twitter gem is the one making the request in order to conditionally customize the encoding. In summary, defining an encoder at the global level makes it difficult to conditionally encode form data based on the current context.

Therefore, this PR adds the ability to pass in an encoder at the instance level when creating an HTTP::FormData::Urlencoded instance, which makes it easier to conditionally encode your form data.

Please let me know if I'm overlooking or missing anything here.


This change is Reviewable

Copy link
Member

@ixti ixti left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @summera)


lib/http/form_data/urlencoded.rb, line 66 at r1 (raw file):

      # @param [#to_h, Hash] data form data key-value Hash
      def initialize(data, encoder = nil)

I would like this to be keyword argument too, just like on FormData.create

@summera
Copy link
Contributor Author

summera commented Mar 3, 2020

@ixti done in 3083334

Copy link
Member

@ixti ixti left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @ixti and @summera)


lib/http/form_data.rb, line 50 at r2 (raw file):

          Multipart.new(data)
        else
          Urlencoded.new(data, encoder: encoder)

This breaks rubocop. Please use Ruby 1.9 Hash syntax instead: :encoder => encoder, same about spec. Other than that I'm happy to merge once CI pass.

- Use ruby 1.9 hash syntax
- Reduce line length in spec
@summera
Copy link
Contributor Author

summera commented Mar 3, 2020

@ixti oops sorry I missed that. Fixed in 518d820

Copy link
Member

@ixti ixti left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @summera)

Copy link
Member

@ixti ixti left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@ixti ixti merged commit 6d9c31b into httprb:master Mar 3, 2020
@summera
Copy link
Contributor Author

summera commented Mar 3, 2020

@ixti thanks for the merge and feedback. Any chance of cutting a new release soon? I'd like to be able to use this with the http gem. I'm happy to submit a PR to http as well, if that's needed.

@ixti
Copy link
Member

ixti commented Mar 4, 2020

No, http gem won't need an update. Will cut form_data release ASAP.

@ixti
Copy link
Member

ixti commented Mar 9, 2020

Released as 2.3.0

@summera
Copy link
Contributor Author

summera commented Mar 9, 2020

@ixti thanks! Much appreciated.

I might be missing something, but on using this with the http gem, wouldn't HTTP#make_request_body (and potentially HTTP#build_request depending on implementation) have to change slightly to allow for an encoder to be passed in? The way it's written now, it's only possible to pass a form data hash.

@ixti
Copy link
Member

ixti commented Mar 9, 2020

@summera Oh. Indeed. Will need to think on how to enhance API of http gem to pass that, right now the most straight forward way is:

form = HTTP::FormData.create(form_data, :encoder => custom_encoder)
HTTP.headers("Content-Type" => form.content_type).post(url, :body => form)

@ixti
Copy link
Member

ixti commented Mar 9, 2020

I'm going to enhance HTTP to accept FormData as is without re-encoding, that will allow to pass form data directly:

HTTP.post(url, :form => HTTP::FormData.new(form_data, :encoder => custom_encoder))

@summera
Copy link
Contributor Author

summera commented Mar 9, 2020

Perfect! Happy to help if you need a hand. Just let me know.

@ixti
Copy link
Member

ixti commented Mar 9, 2020

@summera Sure! Thank you. If you can open a PR that will make HTTP respect FormData as is - will be happy to review and merge.

@summera
Copy link
Contributor Author

summera commented Mar 10, 2020

@ixti 👍 . Done in httprb/http#599

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 21, 2020
Update ruby-http-form_data to 2.3.0.

pkgsrc change: add "USE_LANGAUGES=	# none".


## 2.3.0 (2020-03-08)

* [#29](httprb/form_data#29)
  Enhance HTTP::FormData::Urlencoded with per-instance encoder.
  [@summera][]


## 2.2.0 (2020-01-09)

* [#28](httprb/form_data#28)
  Ruby 2.7 compatibility.
  [@janko][]
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