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

Ensure FormData::Builder field value is a String #5129

Merged
merged 1 commit into from Oct 19, 2017

Conversation

RX14
Copy link
Contributor

@RX14 RX14 commented Oct 15, 2017

See RX14/multipart.cr#7.

field could be passed an integer, which meant the incorrect IO::Memory.new overload was chosen. I chose to fix this by calling to_s on the result, but we could also make this more explicit by restricting to an IO.

end

# Adds a form part called *name*, with data from *io* as the value.
# *Metadata* can be provided to add extra metadata about the file to the
# Content-Disposition header for the form part. Other headers can be added
# using *headers*.
def file(name, io, metadata : FileMetadata = FileMetadata.new, headers : HTTP::Headers = HTTP::Headers.new)
def file(name : String, io, metadata : FileMetadata = FileMetadata.new, headers : HTTP::Headers = HTTP::Headers.new)
Copy link
Member

Choose a reason for hiding this comment

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

why not also restrict io : IO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know :(

Copy link
Member

Choose a reason for hiding this comment

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

I mean, if not putting a type restriction led to a bug, we should always put type restrictions when possible.

@asterite asterite added this to the Next milestone Oct 19, 2017
@asterite asterite merged commit f414e74 into crystal-lang:master Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants