nealrichardson / httptest

A Test Environment for HTTP Requests in R
https://enpiar.com/r/httptest/
Other
79 stars 10 forks source link

start_capturing() should use fully scoped function calls #76

Closed kforner closed 1 year ago

kforner commented 1 year ago

I believe that start_capturing() will fail if the httptest package is not loaded in the search path, because the code it inserts is not fully scoped. For example at https://github.com/nealrichardson/httptest/blob/master/R/capture-requests.R#L90, the call to get_current_redactor() will fail with:

Error in `get_current_redactor()`: could not find function "get_current_redactor"

My guess is that the slight modification of prefixing by httptest:: the calls:

This will allow a clean use of httptest by other R packages, without messing with the search path and thus creating side-effects. What do you think?

nealrichardson commented 1 year ago

I don't feel strongly either way, happy to accept a PR. Do you think you could also add a test that covers the behavior you're trying to support? Probably have to shell out and call R in a separate process since httptest is loaded in its own test suite.

kforner commented 1 year ago

Thanks for your reply.

Do you think you could also add a test ?

I'm fan of TDD, but in this case it's a bit tedious. Provided that it won't alter the current behavior, I would test manually that it works with my use case, and the existing test should check that nothing gets broken. How does that sound?

nealrichardson commented 1 year ago

Sure