Conversation
…g/x/tools and updated vendor dir
…e closed and check if its called
| log.Fatal(err) | ||
| } | ||
|
|
||
| defer func(rows pgx.Rows) { |
There was a problem hiding this comment.
Do you plan to support the t.Cleanup way, I mean, either the detection, or adding them to correct cases ?
I didn't look at all code (past, or added) but I'm wondering.
So I prefer to raise the point, and get a "already supported, RTFM" 😁😅
Of course, it can be addressed in another PR
| var username string | ||
| err = stmt.QueryRowContext(ctx, id).Scan(&username) | ||
| switch { | ||
| case err == sql.ErrNoRows: |
There was a problem hiding this comment.
Non-blocking side remark.
I know you are using the direct sql lib method, but I would have used errors.Is
| case err == sql.ErrNoRows: | |
| case errors.Is(err, sql.ErrNoRows): |
My suggestion is related to all the codebase
| defer func(rows pgx.Rows) { | ||
| rows.Close() | ||
| }(rows) | ||
| } |
There was a problem hiding this comment.
Based on the copy paste you do with small variation, maybe these function could be generated.
But I would understand if you think it overkill
| testdata/pgx_examples/missing_close.go:17:28: Rows/Stmt/NamedStmt was not closed | ||
| testdata/pgx_examples/missing_close.go:26:28: Rows/Stmt/NamedStmt was not closed | ||
| testdata/pgx_examples/missing_close_defer.go:10:26: Rows/Stmt/NamedStmt was not closed | ||
| testdata/pgx_examples/missing_close_defer.go:23:26: Rows/Stmt/NamedStmt was not closed |
There was a problem hiding this comment.
I'm surprised to see such a file
Why not using the / want Go idiomatic comment in the code?
|
Hi @ccoVeille, thank you for your feedback. I checked your review points and and I agree with them but I did not change anything that was existing since I don't know if the author of the repo intended to use certain approaches or not. I only added the feature that I needed to support. Do you think we should change it? |
|
I agree with you. As I said I didn't look at the code base. |
|
Let's wait for @ryanrolds to look at them. But I'm fine with your changes @khchehab, no need to address my comments before merging This one is the one that maybe addressed in another issue if Ryan finds it useful |
|
Thank you for your patience, I've been busy isn't the case anymore. I will work reviewing this into my todos this week. Thank you. |
3 participants
Hi, this is my first contribution and I hope I did this right, please let me know if I missed anything.
When closing a connection/statement/row set I use defer blocks with parameters of what I'm closing. This was not being checked and using
golangci-lintwith this linter reported these errors and that the row set was not being closed.I added this check so that it takes into consideration if a close was done inside a defer block closure that takes in a parameter as well.
I added test cases for successes and failures as well.
I also updated the packages since there was an issue when building it without any changes, due to a panic from one of the indirect dependencies.