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

Handle JSON RPC errors. Resolves issue #672. #673

Merged
merged 3 commits into from
Mar 13, 2018

Conversation

rossmcf
Copy link
Contributor

@rossmcf rossmcf commented Mar 7, 2018

No description provided.

Copy link
Member

@peterbourgon peterbourgon left a comment

Choose a reason for hiding this comment

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

Minus the comment thing, if you're happy, I'm happy :)

// DecodeResponseFunc extracts a user-domain response object from an HTTP
// request object. It's designed to be used in JSON RPC clients, for
// DecodeResponseFunc extracts a user-domain response object from an JSONRPC
// response object. It's designed to be used in JSON RPC clients, for
// client-side endpoints. One straightforward DecodeRequestFunc could be
Copy link
Member

Choose a reason for hiding this comment

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

Oops, looks like a copy/paste flub in the comment! Probably there are others of these, too. Also, does the actual comment from this point on make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting that. I've made some tweaks to the doc comments in that file.

@rossmcf
Copy link
Contributor Author

rossmcf commented Mar 12, 2018

I think this looks tidy now. I'll be using it in anger in the near future, so will feed back if there are any other tweaks needed.

@basvanbeek
Copy link
Member

@peterbourgon before merge I'd like to play a bit with it... can you wait max. a day until merge?

@peterbourgon
Copy link
Member

@basvanbeek I'll wait until you ping me again 👍

Copy link
Member

@basvanbeek basvanbeek left a comment

Choose a reason for hiding this comment

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

LGTM

@peterbourgon
Copy link
Member

Failures are related to the Thrift nonsense, so I'm gonna optimistically merge.

@peterbourgon peterbourgon merged commit feff11c into go-kit:master Mar 13, 2018
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.

3 participants