IanMitchell / discord-notifier

A ruby wrapper around Discord webhooks
MIT License
14 stars 4 forks source link

Image Sending Error - probably affect every kinda file #9

Closed quirinux closed 6 years ago

quirinux commented 6 years ago

First of all, thanks @IanMitchell for this awesome work, it helped me a lot on my projects and help me to save a good bunch of hours.

I got myself trying to send a file with simply sending a File object like described on README page, but once on discord channel it didn't download complaining about wrong file format, it failed for a sort kind of files like PNG, JPG, ZIP and just worked for pure text files.

So I compared the text file that I sent to channel with the original one and for my surprise the sent one had CRLF ending lines chars, even though the original didn't.

At line #33 of form_data.rb file there is a line that adds those CRLF chars into the stream, so I commented this line and it worked like a charm.

My question, do you like a PR to apply that change? Is this line important for anything else? Not sure yet if it impacts on any other file format

quirinux commented 6 years ago

if it does impact anywhere else it'd great if there was a flag to say whether or not CRLF should be added

IanMitchell commented 6 years ago

Oh yikes! Thanks for the detailed report + writeup! Honestly can't remember off the top of my head why I put it there - looking into it now, if I can't figure it out in the next two or three hours I'll drop the line and publish a new version!

IanMitchell commented 6 years ago

Ok, this is weird. The spec for multipart form data specifies it should be using CRLF as a line separator. Not sure what's going on that would be breaking that - trying to test some things out locally.

IanMitchell commented 6 years ago

Yeah, see it working without the line. I'll remove it and publish a new version - my guess is I shouldn't be encoding the file contents with a CRLF, but splitting that out would be more difficult than just dropping the one line, and if it works... ¯_(ツ)_/¯

IanMitchell commented 6 years ago

Aight, 1.0.2 is published and out! Thanks so much for the error report 😁

quirinux commented 6 years ago

Cool, got the version and it looks good now, thanks a mil. i'd read the discord specification, CRLF is there, but i believe that block within form = <<-eof, #19 line, does the trick, if I amn't wrong CRLF should be added as a file/content separator and not for all the content itself, which I think form.encode crlf_newline: true does, in other word I think crlf_newline adds CRLF in the end of everyline line break, even though it is not a properly line break, it's just a guess.

Anyways, thanks again for your awesome work.

Is there anything that I can give you a hand with? Documentation, test cases? Let me know if so

IanMitchell commented 6 years ago

Oh, that makes sense, ok! Thanks for clarifying 😄

Bug reports like these are super helpful actually! I don't use this anywhere (yet) so having people actually testing it and giving feedback is super helpful. If you think additional test cases or documentation would be helpful or something is unclear, I'd absolutely welcome changes!