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 form requests need to use \r\n instead of only \n #74 #118

Closed
wants to merge 3 commits into from

Conversation

rikardq
Copy link

@rikardq rikardq commented Nov 7, 2014

Hello, made a patch for replacing the \n to \r\n for multipart request for issue #74
Added test.

Regards Rikard

Rikard Qvarforth added 2 commits November 6, 2014 08:52
Added function that replaces line feeds \n with carriage return \r in body. It also recalcultes the content length header.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) when pulling 3e1f78a on rikardq:master into 9dec2cc on apiaryio:master.

In order to build Dredd from my repo with npm install
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) when pulling 4e3a6ce on rikardq:master into 9dec2cc on apiaryio:master.

@fosrias
Copy link

fosrias commented Dec 5, 2014

@Almad @tu1ly This seems pretty minor and has been open a while and has tests. Any reason this has no comments by us or is not merged.

@ecordell
Copy link
Contributor

ecordell commented Dec 5, 2014

In my opinion this should be refactored a bit first. The isMultipart and replaceLineFeedInBody are defined in the TransactionRunner class as top level methods but aren't actually related to running transactions (imagine wanting to use the replaceLineFeedInBody elsewhere in the project - you'd need to instantiate a TransactionRunner and then call the method).

They could/should separated into their own modules and simply used by the runner (similar to flattenHeaders and company).

(Just my $0.02 - I can't merge PRs).

@netmilk
Copy link
Contributor

netmilk commented Feb 17, 2015

This is solved in #148

@netmilk netmilk closed this Feb 17, 2015
@netmilk
Copy link
Contributor

netmilk commented Feb 17, 2015

@rikardq It's based on your work, thanks!

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

5 participants