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

correct and use simple sample in FormData::Builder doc #4898

Conversation

icyleaf
Copy link
Contributor

@icyleaf icyleaf commented Aug 30, 2017

No details

@RX14
Copy link
Contributor

RX14 commented Aug 30, 2017

Why? The previous example should work.

@icyleaf
Copy link
Contributor Author

icyleaf commented Aug 31, 2017

It used wrong variable in second param,

file = IO::Memory.new "file contents"
# it be file not io
builder.file("upload", io, FileMetadata.new(filename: "test.txt"))

@RX14
Copy link
Contributor

RX14 commented Aug 31, 2017

I'd prefer it if you simply fixed the variable name, instead of changing the example. Using IO::Memory is easier to understand here, I think. I am biased because I wrote that example though.

@icyleaf
Copy link
Contributor Author

icyleaf commented Aug 31, 2017

In my option, the plain text/binary file often to be use in most of usage, suck like archive file, image file etc. No one will use a IO::Memory.

@RX14
Copy link
Contributor

RX14 commented Aug 31, 2017

It's an example. IO::Memory will be rare in practice but using a file makes the example not be self-contained.

@jhass
Copy link
Member

jhass commented Aug 31, 2017

We shouldn't suggest to use the block-less variant of File.open where we can avoid it.

@icyleaf
Copy link
Contributor Author

icyleaf commented Sep 1, 2017

Updated.

@RX14 RX14 merged commit 4d9aed0 into crystal-lang:master Sep 1, 2017
@RX14 RX14 added this to the Next milestone Sep 1, 2017
@icyleaf icyleaf deleted the fix-variable-name-in-formdata-builder-comment branch September 1, 2017 09:42
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

3 participants