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

Proposal: remove panics from context.Get* methods and return errors explicitly #77

Closed
bkielbasa opened this issue Feb 25, 2020 · 2 comments · Fixed by #78
Closed

Proposal: remove panics from context.Get* methods and return errors explicitly #77

bkielbasa opened this issue Feb 25, 2020 · 2 comments · Fixed by #78
Labels
accepted api change it's used when the public API is going to be changed proposal

Comments

@bkielbasa
Copy link
Collaborator

bkielbasa commented Feb 25, 2020

This proposal is connected to #73.

The goal of the proposal is to remove all panics from the context helper methods and return errors explicitly. That will change the signature of those methods

// from
func (ctx Context) GetString(key interface{}, defaultValue ...string) string {

// to
func (ctx Context) GetString(key interface{}, defaultValue ...string) (string, error) {

Here's an example implementation:

func (ctx Context) GetString(key interface{}, defaultValue ...string) (string, error) {
	if len(defaultValue) > 1 {
       return"", fmt.Errorf("allowed to pass only 1 default value but %d got", len(defaultValue))
    }

	if _, ok := ctx.values[key]; !ok {
		if len(defaultValue) == 1 {
			return defaultValue[0]
		}
		return "", fmt.Errorf("the key %+v does not exist", key)
	}

	value, ok := ctx.values[key].(string)
	if !ok {
		return "", fmt.Errorf("the expected value is not string (%T)", key)
	}
	return value
} 

and in a test step

stringVal, err := ctx.GetString("what-i-want")
if err != nil {
    t.Fatal(err)
}

This code would follow "handle errors explicitly" and general Go conventions but will add some boilerplate to the end user's code. Tradeoffs :)

@bkielbasa bkielbasa added api change it's used when the public API is going to be changed proposal labels Feb 25, 2020
@arberiii
Copy link
Contributor

It look great to me, it would definitely help with debugging and its not that much to the end user's code as you said is Go convention so users are used to it.

@bkielbasa
Copy link
Collaborator Author

Do you have any arguments against the change? If now, I think I'll go for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted api change it's used when the public API is going to be changed proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants