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

Consider using panic #108

Closed
robfig opened this issue Apr 14, 2021 · 8 comments · Fixed by #152
Closed

Consider using panic #108

robfig opened this issue Apr 14, 2021 · 8 comments · Fixed by #152

Comments

@robfig
Copy link
Contributor

robfig commented Apr 14, 2021

Every method in the API returns an error, which makes it unnecessarily cumbersome to use.

For example

// NewFunctionTemplate creates a FunctionTemplate for a given callback.
func NewFunctionTemplate(iso *Isolate, callback FunctionCallback) (*FunctionTemplate, error) {
	if iso == nil {
		return nil, errors.New("v8go: failed to create new FunctionTemplate: Isolate cannot be <nil>")
	}
	if callback == nil {
		return nil, errors.New("v8go: failed to create new FunctionTemplate: FunctionCallback cannot be <nil>")
	}

	cbref := iso.registerCallback(callback)

	tmpl := &template{
		ptr: C.NewFunctionTemplate(iso.ptr, C.int(cbref)),
		iso: iso,
	}
	runtime.SetFinalizer(tmpl, (*template).finalizer)
	return &FunctionTemplate{tmpl}, nil
}

In this case, I would argue that it should panic if either of the parameter are nil, because this is incorrect use of the API and is trivially able to be checked by a caller who might have an Isolate or FunctionCallback of unknown provenance.

To draw a comparison, strings.Repeat panics if an invalid argument is provided, and I have never heard of that being considered problematic. Instead, returning many errors that can never happen either results in callers needing to do one of 3 things:

  1. Add a panic in their code, in which case, what has been saved? This is what I do, unhappily.
  2. Ignore the error. This is bad, because they would need to view the implementation to know which errors are impossible.
  3. Handle the error. This is time wasted and additional code to maintain

Obviously it's a breaking change, but I think it would be a welcome one to remove error returns of this sort from the API.

Thanks for considering.

@rogchap
Copy link
Owner

rogchap commented Apr 14, 2021

Thanks for your thoughts. The intention was to provide better error handling "in the future" so I added (the conventional) return errors from the beginning to avoid backward compatibility issues when we did have an error to return.

I understand the verbosity, but I think this is idiomatic Go, and we should avoid panics if possible 🤷‍♂️

That's just my 2¢; keen to hear other opinions.

@mitar
Copy link

mitar commented May 1, 2021

Alternatively, every existing function could also have a Must* variation, which panics on error. E.g., MustNewFunctionTemplate.

@rogchap
Copy link
Owner

rogchap commented May 1, 2021

This was discussed further here: #118 (comment)

@DeadlySurgeon
Copy link

DeadlySurgeon commented Jun 30, 2021

Rather late, but for the most part you should avoid panics. While they can be nice to use, often it prevents the caller from being able to make a good decision on handling the error, resulting in bad code flow.

Let us imagine if os.Open panics if the file doesn't exist, panicing up the os. ErrNotExist error instead of returning it. What happens if our code is fine with that error, and can handle it, to create the file since it doesn't exist? You then would have to wrap the function in an anonymous function, and pass the caught error out.

file, err := os.Open("file.json")
if err != nil {
    if os.IsNotExist(err) {
        // ...
    }
    return err
}

Becomes closer to this.

var err error
var file os.File
func() {
    defer func() {
        err = recover()
    }
    file = os.Open("file.json")
}()
if err != nil {
    if os.IsNotExist(err) {
        // ...
    }
    return err
}

Also, "to draw a comparison", the Go devs themselves said that comparing the standard library to ideal go code is not recommended. Just because the strings package panics, doesn't mean that you should too. However, @mitar is correct, if you're going to have it panic, have a separate function that has a Must keyword, such as regexp's regexp.MustCompile vs regexp.Compile, so I guess you can have the best of both worlds.

@robfig
Copy link
Contributor Author

robfig commented Jun 30, 2021

I'm not intending to implement or discuss this further, so I'll close this issue.

@dylanahsmith
Copy link
Collaborator

Let us imagine if os.Open panics

Let's avoid using a straw man argument. The issue description already brought up a specific example and wasn't saying this should be done for all errors.

This was discussed further here: #118 (comment)

which discusses another specific example.

@DeadlySurgeon
Copy link

DeadlySurgeon commented Jul 6, 2021

@dylanahsmith Not sure why you're commenting on an already closed and decided issue. And using os.Open as an example isn't a straw man argument, it's just showcasing what if other functions used panics instead of returning errors, plus to draw out the conclusion that not all standard packages panic and instead return errors, since the author used the standard library as an example. I could use literally any other package to demonstrate it. My main concern was that a panic would be added to a function that otherwise shouldn't have it, instead of creating a better suited to have a sister function that is a Must call (which is why I mentioned the regexp package). Because the intent of the issue would result in "a breaking change".

@dylanahsmith
Copy link
Collaborator

Not sure why you're commenting on an already closed and decided issue

I was just trying to highlight that what was decided in the linked to PR comment discussion was different than what was indicated here. Otherwise, I agree with not discussing this further here.

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