Skip to content

Conversation

hiranya911
Copy link
Contributor

@hiranya911 hiranya911 commented Aug 29, 2019

The pattern of making a JSON HTTP request, parsing the response and handling errors is very common in the SDK. This PR introduces a new DoAndUnmarshal() helper function to internal.HTTPClient to implement this pattern. Also includes:

  1. Support for centralized error handling via CreateErrFn and SuccessFn options.
  2. Deprecated the existing ErrParser option, and the CheckStatus() and Unmarshal() functions on the Response.

@hiranya911 hiranya911 changed the title Adding a new internal.JSONHTTPClient API Adding new helper functions to internal.HTTPClient API Aug 30, 2019
URL string
Body HTTPEntity
Opts []HTTPOption
SuccessFn SuccessFn
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kinda weird for both HTTPClient and Request to have so many fields in common. Would it make sense to package them into a separate struct that they each get a pointer to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with that is, callers have to use another level of nesting to specify common properties:

req := &internal.Request{
  CommonSettings: &internal.CommonSettings{
    SuccessFn: mySuccessFn,
  },
}

I'd prefer following over that:

req := &internal.Request{
  SuccessFn: mySuccessFn,
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair.

I do feel like these http methods have probably gotten more complicated than they need to be, and maybe we got some abstraction wrong that's making thing needlessly difficult. But I don't have any specific objections to this change. Certainly nothing worth holding this PR up any longer.

But I think we should brainstorm how to clean this up at some point.

@hiranya911 hiranya911 requested a review from bklimt September 4, 2019 18:22
Copy link
Collaborator

@bklimt bklimt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hiranya911 hiranya911 assigned hiranya911 and unassigned bklimt Sep 11, 2019
@hiranya911 hiranya911 merged commit 3df7cc0 into dev Sep 11, 2019
@hiranya911 hiranya911 deleted the hkj-op-client branch September 11, 2019 20:36
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.

2 participants