sqlclosecheck icon indicating copy to clipboard operation
sqlclosecheck copied to clipboard

Feature/closure param support

Open khchehab opened this issue 1 year ago • 4 comments

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-lint with 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.

khchehab avatar Jun 27 '24 06:06 khchehab

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?

khchehab avatar Jun 28 '24 05:06 khchehab

I agree with you. As I said I didn't look at the code base.

ccoVeille avatar Jun 28 '24 11:06 ccoVeille

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

https://github.com/ryanrolds/sqlclosecheck/pull/37#discussion_r1656557448

ccoVeille avatar Jun 28 '24 11:06 ccoVeille

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.

ryanrolds avatar Jul 29 '24 05:07 ryanrolds