Calls to panic replaced with returning an error

[MarkSonghurst]

Thanks for the review. However, I'm struggling to understand your comment. The original code would call panic with the value of ret and terminate the program (assuming there was no recover to catch the panic), where as my replacement does much the same thing, except it returns the value of ret in an error. Looking at the function and the way it behaves, it would seem there is no available original error to return if SQLGetDiagRec has failed. So the original error within handle cannot be extracted anyway?

[MarkSonghurst]

I can put the apiName into the generated error, maybe that would be sufficient? Something like (line 61) return fmt.Errorf("SQLGetDiagRec failed: ret=%d (original error in %s)", ret, apiName)

[alexbrainman]

The original code would call panic with the value of ret and terminate the program (assuming there was no recover to catch the panic), where as my replacement does much the same thing, except it returns the value of ret in an error.

If you are package user, what would you do with that error? Lets say you have SQL with syntax error, and you get error that says: "SQLGetDiagRec failed: ret=-2". What would your program do to handle this error?

Alex

[MarkSonghurst]

My daemon would print the message in my log at error level and raise an alert for an sysadmin to investigate. Then I would return an HTTP 500 series error message to the client at the other end of my REST API.

[alexbrainman]

My daemon would print the message in my log at error level and raise an alert for an sysadmin to investigate. Then I would return an HTTP 500 series error message to the client at the other end of my REST API.

I don't see how any of these are different from panicking. You could setup your daemon to log stack trace of the crash, send this information to sysadmin to investigate, and restart the program. Why your approach is better?

Alex

[MarkSonghurst]

A panic terminates the program. At any one time my daemon could be processing many requests to many clients. All those requests would be killed by the termination.

[MarkSonghurst]

I'll maintain my own fork of this package. Thanks for your time.

[alexbrainman]

I am sorry, it took me awhile to decide. But if you prefer to keep your own fork, I am fine with that too.

Alex

[MarkSonghurst]

What did you decide? To merge, or not?

[alexbrainman]

I don't have time to discuss now, but I will tell you within one day. If you want to leave it open, I will play with it. Thank you.

Alex

[MarkSonghurst]

I've re-opened the PR, but you will get other unrelated commits I made to my master now. Hopefully you can take just the commits you want easily.

[alexbrainman]

I tried running tests against your changes, but I get this:

# github.com/alexbrainman/odbc
.\errorhandling_test.go:29: cannot use nil as type api.SQLHSTMT in argument to b.Value
.\errorhandling_test.go:43: cannot convert nil to type api.SQLHANDLE
.\errorhandling_test.go:52: cannot use nil as type api.SQLHSTMT in argument to p.BindValue
FAIL    github.com/alexbrainman/odbc [build failed]
# github.com/alexbrainman/odbc
.\errorhandling_test.go:29: cannot use nil as type api.SQLHSTMT in argument to b.Value
.\errorhandling_test.go:43: cannot convert nil to type api.SQLHANDLE
.\errorhandling_test.go:52: cannot use nil as type api.SQLHSTMT in argument to p.BindValue
FAIL    github.com/alexbrainman/odbc [build failed]
# github.com/alexbrainman/odbc
.\errorhandling_test.go:29: cannot use nil as type api.SQLHSTMT in argument to b.Value
.\errorhandling_test.go:43: cannot convert nil to type api.SQLHANDLE
.\errorhandling_test.go:52: cannot use nil as type api.SQLHSTMT in argument to p.BindValue
FAIL    github.com/alexbrainman/odbc [build failed]

Also, please, remove everything unrelated to the subject of this PR from this.

Thank you.

Alex

[MarkSonghurst]

I cannot figure out a way to make my tests support both Linux and Windows. It's due to the type differences in the api directory which Go objects to at compile time. This doesn't work:

    if runtime.GOOS == "windows" {
        v, err = b.Value(api.SQL_NULL_HSTMT, 0)
    } else {
        v, err = b.Value(nil, 0)
    }

I'm closing the pull request; i'll maintain my own fork from here on in, which I guess will now always be Linux specific. Thanks for your time anyway.

[alexbrainman]

You could drop new tests altogether. But keeping private fork is fine with me too.

Alex

[MarkSonghurst]

Hi @alexbrainman If I reopen this PR and do your suggestions made here:

You could drop new tests altogether.

Also, please, remove everything unrelated to the subject of this PR from this.

Will you reconsider this PR? I saw some other people were interested it in #76 so it seems fair for me to share it upstream to you.

[alexbrainman]

Will you reconsider this PR?

I am happy to review it again, but I won't have free time until maybe weekend. And, please, remove everything unrelated to removing panics.

Thank you.

Alex