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

Issue #173 Support uri as first and optional config as second argument #177

Merged
merged 3 commits into from
Feb 19, 2012

Conversation

twilson63
Copy link
Contributor

Implemented uri as first and optional config functionality for Issue #173

Would be great if this format was supported:

request.get(uri, {
  ...
}

so we don't have to put uri in the options config.

Makes it more clear what uri we are trying to connect to.

Also since the uri is mandatory to provide, it makes sense that the options config is for optional config only.


Also added a test and called test-params.js to test this new feature, may not be the best name. :)

Thx

@mikeal
Copy link
Member

mikeal commented Feb 19, 2012

this looks great.

mikeal added a commit that referenced this pull request Feb 19, 2012
Issue #173 Support uri as first and optional config as second argument
@mikeal mikeal merged commit ddc2dbe into request:master Feb 19, 2012
@shimaore
Copy link
Contributor

It looks like this will fail when defaults() is used, defaults() only has (opts, callback).

@twilson63
Copy link
Contributor Author

@shimaore defaults should still work, even though the request method supports uri, options, callback, it should still support (options, callback), (uri), and (options). At least all the test cases in the test-params.js file pass, plus all existing test cases still pass. I also submitted a pull request for supporting the same api for the post, put, head and del short cuts see #180

@mikeal
Copy link
Member

mikeal commented Feb 20, 2012

@twilson63

https://github.com/mikeal/request/blob/master/main.js#L693

defaults wraps all the individual methods and doesn't recognize the new argument pattern.

@twilson63
Copy link
Contributor Author

Got it, working on it, should have something by the end of day.

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

3 participants