search

BrandonRomano / wrecker

A golang request builder for JSON APIs
MIT License
24 stars 0 forks source link

Suggestions for Minor Refactor #13

Closed benpate closed 9 years ago

benpate commented 9 years ago

I have a suggestion for a minor refactor of the API, which will hopefully make the flow a little easier to follow:

  1. Rename wrecker.WreckerRequest to simply wrecker.Request to reduce stutter
  2. Move wrecker.sendRequest() into the wrecker.Request object. I think this functionality is more in line with the request than the root level object.
  3. Add a pointer to the http.Response object into the wrecker.Request struct. This should satisfy the need to inspect and troubleshoot raw responses without complicating the API by adding another return value to each function. It should also simplify request interceptors when it comes time to build those.

I'm happy to do one, two, or all three of these as pull requests, but I figured I should ask before starting. :)

benpate commented 9 years ago

Sorry. One more item:

  1. Per comments on Reddit, it would be good to split Params into separate structures for URL params and Form parameters.
BrandonRomano commented 9 years ago

Rename wrecker.WreckerRequest to simply wrecker.Request to reduce stutter

Yes definitely, I agree with this... Much better!

Move wrecker.sendRequest() into the wrecker.Request object. I think this functionality is more in line with the request than the root level object.

So in it's current state, yes it is more in line with the request object. After we add the request interceptor though that function is going to depend on the wrecker instance. Logically, I think of it as wrecker is a client wrapper that provides a cleaner interface.

I understand why you want this change, but I think as the library evolves we'll see that it actually should be associated to the wrecker instance... Let's hold off on this change for now, and revisit this in a month or so if we still think the same way!

Add a pointer to the http.Response object into the wrecker.Request struct. This should satisfy the need to inspect and troubleshoot raw responses without complicating the API by adding another return value to each function. It should also simplify request interceptors when it comes time to build those.

Hmm... I'm not entirely sure on this one. I'm more leaning towards a solution that I've proposed in #9. Basically, firing off requests will return the response. I think it would be a little odd if the user had to dig into the request, to get the response... no?

I think the PR that I proposed provides the solution of exposing the response, but I think that would be a little bit of a better way of doing it then putting it in the request.

Per comments on Reddit, it would be good to split Params into separate structures for URL params and Form parameters.

Yeah I saw this one... We should probably use WithUrlParam and WithFormParam!

BrandonRomano commented 9 years ago

Also, let me know your thoughts on these, and we can break them up into smaller issues!

benpate commented 9 years ago

Hey Brandon,

I’m posting a new branch with the rename.

I think I agree with you on #2 and #3. It’s better to leave it as it is for now.

I’ll get the request interceptor branch updated and over into your repo later today.

Talk to you soon!

— Ben

On Jun 22, 2015, at 12:12 PM, Brandon Romano notifications@github.com wrote:

Also, let me know your thoughts on these, and we can break them up into smaller issues!

— Reply to this email directly or view it on GitHub https://github.com/BrandonRomano/wrecker/issues/13#issuecomment-114204682.

BrandonRomano commented 9 years ago

Hey @benpate, just broke this suggestion into actionable issues (linked above)... Going to close this issue in favor of them!