search

aurelia / http-client

A simple, restful, message-based wrapper around XMLHttpRequest.
MIT License
62 stars 59 forks source link

feat request-builder: add withSkipContentProcessing helper #82

Closed sujesharukil closed 9 years ago

sujesharukil commented 9 years ago

add withSkipContentProcessing helper to preserve content and prevent json stringification.

fixes issue #57

EisenbergEffect commented 9 years ago

@bryanrsmith Can you review?

EisenbergEffect commented 9 years ago

@bryanrsmith CLA is good if you want to merge.

alexanderchr commented 9 years ago

I think the helper should be called skipContentProcessing or maybe withoutContentProcessing. 'With' makes no sense in this case.

Also, the modifications to dist/ should be removed as these files are updated with version bumps.

sujesharukil commented 9 years ago

I qas trying to follow convention to have "with" prefix to helpers. Or am I mistaken on convention? I personally like "skipContentProcessing". I can make the changes and resubmit?

alexanderchr commented 9 years ago

I don't think there's any convention like that on helper naming as there are currently some that does not start with as.

Just amend the commit and force push to your branch and this pr will update itself :)

bryanrsmith commented 9 years ago

I agree with @alexanderchr. skipContentProcessing reads nicely. I think the other helpers are just named using the with prefix because they happened to work well with it.

It also looks like there are some merge conflicts that will need to be resolved.

Other than those adjustments this looks good to me. Thanks for contributing!

sujesharukil commented 9 years ago

Got it. Will do it soon.

  1. Merge latest
  2. Get rid of dist changes
  3. Make changes and force push.

Just a quick question on dist folder.. Shouldn't that be in gitignore?

EisenbergEffect commented 9 years ago

the dist folder is needed for actual releases.

sujesharukil commented 9 years ago

Got it

sujesharukil commented 9 years ago

There you go Gentlemen, updated as per review. Thanks :)

EisenbergEffect commented 9 years ago

@bryanrsmith Please review at your earliest convenience.