search

alexbrainman / odbc

odbc driver written in go
BSD 3-Clause "New" or "Revised" License
352 stars 140 forks source link

Replaced calls to panic with returned error #104

Closed MarkSonghurst closed 6 years ago

MarkSonghurst commented 6 years ago

As requested in #77

MarkSonghurst commented 6 years ago

Thanks for the review, I have made the requested change.

alexbrainman commented 6 years ago

LGTM

Thank you.

Alex

MarkSonghurst commented 6 years ago

@alexbrainman I think something strange has happened - when I pull master, I cannot see my changes and the panic calls remain? Also browsing in github shows the same - for example on line 61: https://github.com/alexbrainman/odbc/blob/master/error.go

alexbrainman commented 6 years ago

I think something strange has happened - when I pull master, I cannot see my changes and the panic calls remain? Also browsing in github shows the same - for example on line 61: https://github.com/alexbrainman/odbc/blob/master/error.go

I can see your change here https://github.com/alexbrainman/odbc/commit/bfee230746b69a0370e3726f11e92faea027ca11

And when I pull from master I can see your changes on my computer all right.

Alex

MarkSonghurst commented 6 years ago

@alexbrainman Thanks - but it's still wrong for me:

[msonghurst@deimos test]$ git clone git@github.com:alexbrainman/odbc.git
Cloning into 'odbc'...
remote: Counting objects: 365, done.
remote: Compressing objects: 100% (12/12), done.
remote: Total 365 (delta 2), reused 4 (delta 1), pack-reused 352
Receiving objects: 100% (365/365), 136.69 KiB | 331.00 KiB/s, done.
Resolving deltas: 100% (230/230), done.
[msonghurst@deimos test]$ grep -R panic *
odbc/column.go:     panic(fmt.Errorf("do not know how wide column of ctype %d is", ctype))
odbc/column.go:     panic(fmt.Errorf("wrong column #%d length %d returned, %d expected", idx, c.Len, c.Size))
odbc/driver.go:     panic(err)
odbc/error.go:          panic(fmt.Errorf("SQLGetDiagRec failed: ret=%d", ret))
odbc/mssql_test.go:             panic(err)

Any suggestions please? I have deleted my fork as I now want to use yours, but I can't see that causing this issue.

MarkSonghurst commented 6 years ago

Also if you click on https://github.com/alexbrainman/odbc/blob/master/error.go can you see line 61 with the panic still?

alexbrainman commented 6 years ago

Also if you click on https://github.com/alexbrainman/odbc/blob/master/error.go can you see line 61 with the panic still?

But your PR did not change line 61 in error.go. See for yourself there https://github.com/alexbrainman/odbc/pull/104/files

Alex

MarkSonghurst commented 6 years ago

Ah, you are right! My bad, sorry. Not a problem. Thanks for your time.

MarkSonghurst commented 6 years ago

@alexbrainman I think some of my panic/error changes got lost when I squashed commits. I'll raise another PR with the missing changes in. I'm very sorry for the inconvenience.

alexbrainman commented 6 years ago

I'll raise another PR with the missing changes in.

Sounds good.

I'm very sorry for the inconvenience.

No worries.

Alex