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

r2.New() doesn't invoke http.NewRequest() #141

Open
dhermes opened this issue Nov 15, 2019 · 1 comment
Open

r2.New() doesn't invoke http.NewRequest() #141

dhermes opened this issue Nov 15, 2019 · 1 comment
Labels
v3.x.y Version 3 series

Comments

@dhermes
Copy link
Contributor

dhermes commented Nov 15, 2019

The current approach is to just manually construct an instance via http.Request{ ... }, but this misses out on goodies like setting the default protocol or the host.

We don't strictly need to invoke it in the same way, but any option that would correspond to one of the four arguments of NewRequestWithContext (ctx, method, url, body) should have similar behavior.

(This came about because @PaulMH wrote some perfectly normal looking r2 request code that didn't set the content length on a post.)

@dhermes
Copy link
Contributor Author

dhermes commented Dec 12, 2019

Based on some digging I've done for #154, ISTM we may be better off removing the embedded http.Request from r2.Request and going back to a Request() helper that creates a request on the fly. If not, we have to worry about mutations to an http.Request that should have the same side affects that changes to http.NewRequest() arguments would have (we could do this just by calling http.NewRequest() and copying all the fields over, but this could be costly performance wise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3.x.y Version 3 series
Projects
None yet
Development

No branches or pull requests

1 participant