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

fix requester bug #727

Merged
merged 1 commit into from
Dec 5, 2013
Merged

fix requester bug #727

merged 1 commit into from
Dec 5, 2013

Conversation

jchris
Copy link
Contributor

@jchris jchris commented Dec 5, 2013

the ability to wrap the callback wasn't available for verbs aside from GET and DELETE, so I made it work for all verbs

…ethods

It wasn't working on PUT and POST before.
@jhs
Copy link
Contributor

jhs commented Dec 5, 2013

Just wanted to say, requests's .defaults() is (1) awesome, but (2) a bit unevenly supported. It was my inspiration to write defaultable.

https://github.com/nodejitsu/defaultable

@benatkin
Copy link
Contributor

benatkin commented Dec 5, 2013

I read the update and the relevant parts of the code, and it looks good. I'm confident that it doesn't break anything. requester is however undocumented in the README. It would be nice for it to hint when it's useful, and I'm not sure. Any examples of use (real or hypothetical)? A failing test would also be nice.

@jchris
Copy link
Contributor Author

jchris commented Dec 5, 2013

Here is a gist with the way I was using it when I noticed the bug:

https://gist.github.com/jchris/7801297

On Wed, Dec 4, 2013 at 11:30 PM, Ben Atkin (I accept patches by email.) <
[email protected]> wrote:

I read the update and the relevant parts of the code, and it looks good.
I'm confident that it doesn't break anything. requester is however
undocumented in the README. It would be nice for it to hint when it's
useful, and I'm not sure. Any examples of use (real or hypothetical)? A
failing test would also be nice.


Reply to this email directly or view it on GitHubhttps://github.com//pull/727#issuecomment-29876880
.

Chris Anderson
http://jchrisa.net
http://www.couchbase.com

@mikeal
Copy link
Member

mikeal commented Dec 5, 2013

was _requester already supported? if it's _ then it's private, and if we're opening it up for public use we should remove the _.

@jchris
Copy link
Contributor Author

jchris commented Dec 5, 2013

It is supported as a the second argument to the defaults function, with the
_requester thing just some internal state that I think @polotek added
8cf019c

On Thu, Dec 5, 2013 at 8:17 AM, Mikeal Rogers [email protected]:

was _requester already supported? if it's _ then it's private, and if
we're opening it up for public use we should remove the _.


Reply to this email directly or view it on GitHubhttps://github.com//pull/727#issuecomment-29911449
.

Chris Anderson
http://jchrisa.net
http://www.couchbase.com

mikeal added a commit that referenced this pull request Dec 5, 2013
@mikeal mikeal merged commit 8e34457 into request:master Dec 5, 2013
@mikeal
Copy link
Member

mikeal commented Dec 5, 2013

we'll want to revisit this use case for 3.0, i think there's a better way to enable this. until then, let's hold off on documenting it.

nylen pushed a commit to nylen/request that referenced this pull request Oct 17, 2014
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.

5 participants