sqrl icon indicating copy to clipboard operation
sqrl copied to clipboard

Add RowsScanner interface

Open ajpetersons opened this issue 6 years ago • 3 comments

Currently it is impossible to fully unit test code that is using sqrl since Query method requires *sql.Rows as return value. I am proposing to add RowsScanner interface that would allow *sql.Rows to be replaced by another struct that implements required methods.

type Queryer interface {
    Query(query string, args ...interface{}) (RowsScanner, error)
}

As far as I can tell, RowsScanner could look something like this:

type RowsScanner interface {
    Columns() ([]string, error)
    Next() bool
    Close() error
    Err() error
    RowScanner
}

ajpetersons avatar Oct 05 '18 12:10 ajpetersons

Hi @ajpetersons! Thanks for the idea! Sorry for taking so much time. This change needs some thought, though. The purpose of sqrl is to help building SQL queries, not running them. IMHO, this part is more like "syntactic sugar": you provide your *sql.DB connection, sqrl generates a query, runs it and returns familiar sql.* objects. Just to support interface of *sql.DB and keep return types consistent. Personally, I'd be surprised to get sqrl.RowsScanner instead of *sql.Rows I expected from *sql.DB calls.

What do you think?

elgris avatar Oct 20 '18 19:10 elgris

What I am proposing (in my mind) is to have something similar to QueryRow method. At the moment QueryRow returns an interface which is no problem to operate with the same way as if it was *sql.Row. As long as the proposed RowsScanner interface is complete, you could use the interfaced object anywhere you would normally use *sql.Rows as it does not have exported fields, only methods.

ajpetersons avatar Oct 21 '18 13:10 ajpetersons

All right, thanks for answering. I expressed my concerns in the comments in the PR. It looks to me that it may end up in a bigger refactoring. I'd keep it open for some time to collect opinions and votes, but I'd rather not merge it as it currently is.

elgris avatar Oct 27 '18 10:10 elgris