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

Example StringSvc3 Has Endpoint Code Mismatch #772

Closed
mattc41190 opened this issue Oct 4, 2018 · 2 comments
Closed

Example StringSvc3 Has Endpoint Code Mismatch #772

mattc41190 opened this issue Oct 4, 2018 · 2 comments

Comments

@mattc41190
Copy link

mattc41190 commented Oct 4, 2018

In the third section of the example stringsvc application the guide tells the user that the following snippet is the correct definition for a proxied version of the Uppercase service:

func (mw proxymw) Uppercase(s string) (string, error) {
	response, err := mw.uppercase(uppercaseRequest{S: s})
	if err != nil {
		return "", err
	}
	resp := response.(uppercaseResponse)
	if resp.Err != "" {
		return resp.V, errors.New(resp.Err)
	}
	return resp.V, nil
}

Since mw.uppercase returns an Endpoint and Endpoints must recieve two args this cannot be correct.

Meanwhile, the code in GitHub has this definition:

func (mw proxymw) Uppercase(s string) (string, error) {
	response, err := mw.uppercase(mw.ctx, uppercaseRequest{S: s})
	if err != nil {
		return "", err
	}

	resp := response.(uppercaseResponse)
	if resp.Err != "" {
		return resp.V, errors.New(resp.Err)
	}
	return resp.V, nil
}

The example code in the guide should likely include ctx as part of the struct and ctx should be included in the response variable creation.

There was recently some work done to remove ctx from the earlier examples in stringsvc1. Perhaps there is already work being done in this area?

Sources:

http://gokit.io/examples/stringsvc.html#calling-other-services
https://github.com/go-kit/kit/blob/master/examples/stringsvc3/proxying.go#L69-L90

@jstoja
Copy link

jstoja commented Oct 18, 2018

I think that the rationnale is that, context is not needed in these examples, and it has been removed from the StringSvc1 example to avoid clutter and make it easier to understand.
I guess we could just remove either context from the StringSvcvX examples or, actually add the context bit on the website to explain why it could be used and how to pass it.

I'd be in favor of removing it in all the StringSvc since it's covered in the "Advanced topics" area on the same page.

@peterbourgon
Copy link
Member

#1197

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants