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

Decode shouldn't panic #7

Closed
themihai opened this issue Jul 25, 2016 · 4 comments
Closed

Decode shouldn't panic #7

themihai opened this issue Jul 25, 2016 · 4 comments
Assignees
Milestone

Comments

@themihai
Copy link

It seems that Decode panics if it receives invalid input. It is not idiomatic go and should be refactored using return errors. Otherwise at least the behaviour should be documented so that the caller can guard it with recover.

@deankarn
Copy link

I think if I can get #6 working the panic will just go away; however the panic is there to ensure that a bad type is not passed in for decoding because it's of type interface{} you could pass anything.

I was trying to avoid mixing decoding errors and a top level error so:

err := Decode(stuff)

// was trying to avoid this...
if err == form.ErrBadInputType {
// handle error....
}

if err != nil {
// handle decode errors
}

@deankarn deankarn self-assigned this Jul 25, 2016
@deankarn deankarn added this to the v1 milestone Jul 25, 2016
deankarn pushed a commit that referenced this issue Jul 26, 2016
deankarn pushed a commit that referenced this issue Jul 26, 2016
@deankarn
Copy link

Hey @themihai this change has been made is in Release 2.0.0

if there are any issues please don't hesitate to reopen.

@powerman
Copy link

powerman commented Apr 4, 2018

I'm not sure you've fixed right thing here.

Decoder may panic when called with nil or non-pointer param because calling it this way is an obvious bug in caller's code, and panic is here exactly for such case.

At same time decoder must not panic on wrong user input (field names with unmatched brackets) - this must be handled by returning an error.

@powerman
Copy link

powerman commented Apr 5, 2018

BTW, looks like there a couple more errors should be converted to panics (because they're bugs in caller's code):

  • "Unsupported Map Key …"
  • errArraySize

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