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

Avoid use of panic #34

Open
vcabbage opened this issue Jan 13, 2016 · 6 comments
Open

Avoid use of panic #34

vcabbage opened this issue Jan 13, 2016 · 6 comments

Comments

@vcabbage
Copy link

I have some tests that verify my application behaves properly when there are database issues. I found that on head these tests not only fail but end the test run due to the call to a call to logger.Fatal (

logger.Fatal("Could not query Postgres version")
), which eventually calls panic in logxi.

It's recommended to avoid exposing panics to consumer of a package and instead return an error so the consumer can choose how to deal with it. https://github.com/golang/go/wiki/PanicAndRecover#usage-in-a-package

On a related note, I noticed there are also some uses of log.Fatalf in sqlx-runner/db.go and sqlx-runner/tx.go. These call os.Exit(1) and should be avoid for similar reasons.

@mgutz
Copy link
Owner

mgutz commented Jan 14, 2016

Those are intentional. The first link's code ensures that interpolation is not used on a database where escape sequences are allowed in strings. The other two calls only occur in development mode when dat.Strict is set to true and only when a transaction cannot be committed or rolled back. In production mode, the error is returned.

@vcabbage
Copy link
Author

Understood, but why does it panic? The consumer may want to know about and handle the error.

@heynemann
Copy link

I love dat, but I'm 100% with @vcabbage. It's being somewhat annoying having to handle the panics/fatals in my code. It would be so much easier to just handle an error (and I believe more idiomatic as well, the standard library very rarely panics).

Even so, thank you very much @mgutz for the awesome library.

@mgutz
Copy link
Owner

mgutz commented Feb 15, 2017

Which version are you using? V1 or V2?

@heynemann
Copy link

V1.

@mgutz
Copy link
Owner

mgutz commented Feb 15, 2017

Most of the panics have been converted to errors in V2. A proper package without panics requires a moderate refactor and version bump. That's down the future when I find time.

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

No branches or pull requests

3 participants