search

DATA-DOG / go-sqlmock

Sql mock driver for golang to test database interactions
Other
6.16k stars 407 forks source link

Improve panic output with rows.AddRow #325

Closed co60ca closed 1 year ago

co60ca commented 1 year ago

Proposal

https://github.com/DATA-DOG/go-sqlmock/blob/b2f0b45ee89df7dfd29e32a5f9bf087791c9a61f/rows.go#L169 This panic can be confusing, its simple enough to debug if you know what you're doing but if you pass a string withlen() = 25 it will of course panic if the developer does not spreading. Like with: rows.AddRow(v) vs rows.AddRow(v...) (Spreading will cause another issue with []string but I digress.

The former will compile but will panic. I wonder if you'd accept it to say "Expected number of values to match number of columns, expected %d, actual %d" to at least hint.

I wonder if this is acceptable or if it would break backwards compatibility in an unacceptable way. If you'd accept this change I'd gladly provide a PR.

Use-cases

Changing the panic message should simplify debugging sqlmock during development.

Otherwise we can close.

JessieAMorris commented 1 year ago

This sounds like a pretty reasonable change. It’s possible it might break something, but a panic isn’t something that would be checked against super frequently so I see it as a pretty low risk change.

co60ca commented 1 year ago

@JessieAMorris please consider my PR and thank you.