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

Multipart.add_file_content_with_name (issue #69) #71

Merged
merged 1 commit into from Oct 4, 2017
Merged

Multipart.add_file_content_with_name (issue #69) #71

merged 1 commit into from Oct 4, 2017

Conversation

visciang
Copy link

@visciang visciang commented Oct 1, 2017

This PR extend Multipart functions with:

  • Multipart.add_file_content
  • Multipart.add_file_content_with_name

Those are a variation on Multipart.add_file and * Multipart.add_file_with_name. They accept the file_content binary in place of file_path.

Note
Multipart was strictly based on hackney multipart implementation and so the {:multipart, multiparts} request body form was fully compatible with :hackney.request.

With this extension we diverge from hackney breaking the request body compatibility.
This is the reason why I changed Maxwell.Adapter.Hackney.send_multipart callback to use a Util.multipart_encode.

Another option is to open a PR to hackney in the same direct of this one.

In my opinion, given that only hackney natively support multipart body, it's better to use for all the adapter the Maxwell multipart encoder. It is consistent among all adapter library and easier to maintain.

If you agree with me, a further improvement could be refactoring out send_multipart callback from all the adapters and call Util.multipart_encode in adapter.ex call before passing down to send_direct (as I did in this PR for hackney). Let me know.

@coveralls
Copy link

coveralls commented Oct 1, 2017

Coverage Status

Coverage increased (+0.07%) to 98.623% when pulling 4aef0ea on visciang:multipart_file_content into cb3b163 on zhongwencool:master.

@zhongwencool zhongwencool merged commit 4b49716 into zhongwencool:master Oct 4, 2017
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.

None yet

3 participants