Querystring option

[csainty]

#172

Took a stab at adding support for passing in a querystring option parameter.
First contribution to the project and I am still pretty new to Node, so happy to talk over any problems in code.

It got a bit interesting with oAuth, the oAuth code supports pulling its parameters from the body or the querystring, with the querystring taking preference with a basic truthy test. This looked wrong to me, but the change I have made would cause a change in functionality in certain circumstances.

Also included is a js based test runner if you want to point the npm test script at it. Running windows here, so needed something platform agnostic.

[mikeal]

the query option should be named qs to match the composable API.

also, it's unsafe to set properties on self.uri because the underlying url parsing library in core is subject to change. it would be better to simply re-compose the url string with the new querystring and reset self.uri to a new url.parse(newurl).

oAuth is dark voodoo that nobody really understands. it's unclear to me how signing should work if both the querystring and form body are present (which is a totally legal HTTP request). i would just leave oAuth alone and once someone is trying to use both querystring arguments and form bodies we can see how different servers react and tweak as necessary.

[csainty]

I'll need to have a think about the self.uri side of things. I want to support appending qs {} values to an existing querystring that was part of the uri.
I'll take a look later tonight after I have a think on it. Spotted a bug as well that I will fix for the next update.

[csainty]

Ok I went away and thought about it a bit. The change is now self contained and much cleaner while still passing all tests.
The only piece I am unhappy with is where it formats then parses the uri object. It feels like too much work for simply merging querystrings.
At this stage I do not see a good work around other than removing the dependency on self.uri as an actual uri object and instead making it our own structure that can be tweaked without the double processing.

[mikeal]

the patch looks great now but i'm wondering what the run.js is?

we have a shell script that execs all the tests and can be executed using npm test.

[csainty]

The current script is a bash script. I'm on windows here and it doesn't run. I can drop it from the commit if you like, but thought it might be of interest.

[mikeal]

good point.

i'll take it but please remove the bash script, change package.json to use this, and change it to running a static list of files because people tend to keep tests that aren't passing in that directory without checking them in.

[csainty]

Done.

Keep in mind this commit is now behind master a little and there are extra tests that will need to be added to static list in run.js.

Rebase etiquette is something I still am a little unsure of, I read conflicting things about whether I should rebase, merge or leave it to the project owner a this point.

[mikeal]

if it merged cleanly i won't rebase, i'll just merge it now and then add the other tests in myself.