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

Add RowsScanner interface #24

Open
ajpetersons opened this issue Oct 5, 2018 · 3 comments
Open

Add RowsScanner interface #24

ajpetersons opened this issue Oct 5, 2018 · 3 comments

Comments

@ajpetersons
Copy link

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
}
@elgris
Copy link
Owner

elgris commented Oct 20, 2018

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?

@ajpetersons
Copy link
Author

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.

@elgris
Copy link
Owner

elgris commented Oct 27, 2018

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.

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

2 participants