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

toolkit: convert WFN.Value to value receiver #1278

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

crozzy
Copy link
Contributor

@crozzy crozzy commented Mar 6, 2024

When using the go sql driver WFN will not be recognized as a driver.Valuer (only *WFN). As WFN is direct value in claircore's structs this makes it fiddly, the change allows both pointer and value to benefit from Value().

@crozzy crozzy requested a review from a team as a code owner March 6, 2024 19:55
@crozzy crozzy requested review from hdonnay and removed request for a team March 6, 2024 19:55
When using the go sql driver WFN will not be recognized as a
driver.Valuer (only *WFN). As WFN is direct value in claircore's structs
this makes it fiddly, the change allows both pointer and value to
benefit from Value().

Signed-off-by: crozzy <[email protected]>
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.43%. Comparing base (befa444) to head (53712a5).
Report is 4 commits behind head on main.

❗ Current head 53712a5 differs from pull request most recent head 3542340. Consider uploading reports for the commit 3542340 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1278   +/-   ##
=======================================
  Coverage   56.42%   56.43%           
=======================================
  Files         233      233           
  Lines       15355    15355           
=======================================
+ Hits         8664     8665    +1     
+ Misses       5806     5803    -3     
- Partials      885      887    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hdonnay
Copy link
Member

hdonnay commented Mar 7, 2024

I think this was the case in the initial migration into the toolkit module and that broke things -- double-check the git log

@crozzy
Copy link
Contributor Author

crozzy commented Mar 7, 2024

It looks like it was added here, and existed in the old package at the same time here, with the value receiver, I can't seem to find any specifics about why the method was changed.

If I'm thinking about this correctly this should never be an issue with pgx and issues will only surface when implementing a datastore that leverages the sql/driver lib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants