Skip to content

feat(civil): implement database/sql.Scanner|Valuer (#1145)#11808

Merged
gcf-merge-on-green[bot] merged 5 commits intogoogleapis:mainfrom
derekperkins:civil-scanner
Mar 19, 2025
Merged

feat(civil): implement database/sql.Scanner|Valuer (#1145)#11808
gcf-merge-on-green[bot] merged 5 commits intogoogleapis:mainfrom
derekperkins:civil-scanner

Conversation

@derekperkins
Copy link
Copy Markdown
Contributor

Fixes #1145. I wasn't sure how to test these in the context of this repo.

Questions:

  • I tried to be comprehensive in the Scan functions, I'm not sure exactly what types of values to expect
  • For the Valuer, I assumed that most/all databases should accept the ISO string values, but that may not be globally true

@derekperkins derekperkins requested a review from a team March 12, 2025 02:15
@quartzmo
Copy link
Copy Markdown
Member

@derekperkins Thanks for opening this PR. I'm trying to direct it to engineers directly involved with database products. Greetings from SLC, UT btw.

@quartzmo
Copy link
Copy Markdown
Member

cc @rahul2393

@quartzmo quartzmo self-assigned this Mar 12, 2025
@quartzmo quartzmo requested a review from rahul2393 March 12, 2025 17:37
@derekperkins
Copy link
Copy Markdown
Contributor Author

Greetings from SLC, UT btw.

Thanks, you too! You should come to the monthly meetup some time, we usually have ~20-30 people https://www.meetup.com/utahgophers/

@quartzmo
Copy link
Copy Markdown
Member

Thanks for the link! Just joined utahgophers/ using my personal acct.

@quartzmo
Copy link
Copy Markdown
Member

@derekperkins Looks like your PR got approval! 🎉 Please click "Update branch" and I'll restart the CI, then merge when green.

@quartzmo
Copy link
Copy Markdown
Member

@derekperkins Oops, sorry but it looks like I need to approve this too. So, that means a harder look and I saw your question about testing. Are you able to run civil/civil_test.go and add coverage for your Scanner|Valuer funcs?

@quartzmo
Copy link
Copy Markdown
Member

@shollyman PTAL:

Questions:

  • I tried to be comprehensive in the Scan functions, I'm not sure exactly what types of values to expect
  • For the Valuer, I assumed that most/all databases should accept the ISO string values, but that may not be globally true

@derekperkins
Copy link
Copy Markdown
Contributor Author

@quartzmo I went ahead and added some tests. They call the functions directly, vs routing through database/sql, but that seems sufficient for here.

Copy link
Copy Markdown
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay getting to this, LGTM.

@shollyman shollyman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 19, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 19, 2025
@shollyman shollyman added kokoro:force-run Add this label to force Kokoro to re-run the tests. automerge Merge the pull request once unit tests and other checks pass. labels Mar 19, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 19, 2025
@gcf-merge-on-green gcf-merge-on-green bot merged commit cbe4419 into googleapis:main Mar 19, 2025
8 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Mar 19, 2025
@derekperkins derekperkins deleted the civil-scanner branch March 19, 2025 22:45
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

Successfully merging this pull request may close these issues.

civil: support sql.Scanner and driver.Valuer

5 participants