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

Set Content-Type on non-file parts #9

Closed
wants to merge 1 commit into from

Conversation

ixti
Copy link
Member

@ixti ixti commented Jan 18, 2017

Resolves #5

@abotalov
Copy link
Contributor

abotalov commented Jan 18, 2017

It seems to me that this PR:

  1. Sets Content-Type for all parts without allowing a user of the gem to create a part without Content-Type.
    But RFC 7578 says that "Each part MAY have an (optional) "Content-Type" header field, which defaults to "text/plain"". So IMO this gem should either:
    • set Content-Type to text/plain for non-file parts
    • not add such header by default
      I think 2nd variant is better as API (see proposed variant below) becomes easier to use.
      Regardless of the chosen solution, a user should be able to say that header shouldn't be added for a part (as this header is said to be optional in RFC).
  2. Requires a user to implement a class that responds to #mime_type and #to_s to use the ability of overriding Content-Type.
    IMO a user of the gem shouldn't be required to implement some class. I think a library should provide such class directly.

All in all, I think API should allow usage like (fully backwards-compatible):

form = HTTP::FormData.create({
  :username     => "ixti", # creates a part without Content-Type
  :avatar_file  => HTTP::FormData::File.new("/home/ixti/avatar.png")
})
form = HTTP::FormData.create({
  :username     => HTTP::FormData::Part.new("ixti"), # creates a part without Content-Type, same as previous
  :avatar_file  => HTTP::FormData::File.new("/home/ixti/avatar.png")
})
form = HTTP::FormData.create({
  :username     => HTTP::FormData::Part.new("ixti", :content_type => "application/json"), # creates a part with Content-Type
  :avatar_file  => HTTP::FormData::File.new("/home/ixti/avatar.png")
})

@ixti
Copy link
Member Author

ixti commented Jan 18, 2017

Hm. Then I misunderstood issue description. Agree with your proposals. Will try to fix PR as soon as will have time for that

@abotalov
Copy link
Contributor

@ixti I can implement this feature myself too if you would like me to do it 😄

@ixti
Copy link
Member Author

ixti commented Jan 18, 2017

@abotalov sure

@ixti
Copy link
Member Author

ixti commented Jan 22, 2017

Closing in favour of #10

@ixti ixti closed this Jan 22, 2017
@ixti ixti deleted the fix/content-type-on-non-file-multipart branch January 22, 2017 01:23
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