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

Implement request callbacks, closes #408 #409

Merged
merged 2 commits into from Nov 18, 2016
Merged

Implement request callbacks, closes #408 #409

merged 2 commits into from Nov 18, 2016

Conversation

@gaborcsardi
Copy link
Contributor

@gaborcsardi gaborcsardi commented Nov 13, 2016

More details in #408. There is an HTTP mocking implementation that uses this, in the https://github.com/gaborcsardi/httrmock package.

Copy link
Member

@hadley hadley left a comment

Looks good.

However, I do wonder if the interface would be a little cleaner if there were multiple callbacks, rather than calling the same callback with different arguments.

set_callback("request", ...)
set_callback("response", ...)

@@ -122,6 +122,12 @@ request_prepare <- function(req) {

request_perform <- function(req, handle, refresh = TRUE) {
stopifnot(is.request(req), inherits(handle, "curl_handle"))

orig_req <- req
Copy link
Member

@hadley hadley Nov 13, 2016

Can you this and the block below up into a small helper function that lives in callback.r?

Copy link
Contributor Author

@gaborcsardi gaborcsardi Nov 13, 2016

Sure. Btw. I was also thinking that probably we should use the request as it is sent out, i.e. after request_prepare, no?

Copy link
Member

@hadley hadley Nov 14, 2016

Yes, I'd say so.

@gaborcsardi
Copy link
Contributor Author

@gaborcsardi gaborcsardi commented Nov 13, 2016

Yes, two callbacks is definitely cleaner, I'll do that, too.

@gaborcsardi gaborcsardi mentioned this pull request Nov 16, 2016
@gaborcsardi
Copy link
Contributor Author

@gaborcsardi gaborcsardi commented Nov 18, 2016

@hadley OK, updated, PTAL.

@hadley
Copy link
Member

@hadley hadley commented Nov 18, 2016

LGTM - can you please add a bullet to NEWS?

@gaborcsardi
Copy link
Contributor Author

@gaborcsardi gaborcsardi commented Nov 18, 2016

Added NEWS.

@hadley hadley merged commit 8fa4a71 into r-lib:master Nov 18, 2016
1 check passed
@hadley
Copy link
Member

@hadley hadley commented Nov 18, 2016

Thanks!

jiwalker-usgs added a commit to jiwalker-usgs/httr that referenced this issue Jul 24, 2017
* Implement request callbacks, closes r-lib#408

* NEWS about callbacks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants