-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 Serialize and Deserialize support #1089
Conversation
It's not clear to me why the build is failing because:
What am I missing? |
b15a910
to
1d8534a
Compare
Codecov ReportBase: 46.09% // Head: 46.80% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1089 +/- ##
==========================================
+ Coverage 46.09% 46.80% +0.70%
==========================================
Files 11 12 +1
Lines 1499 1534 +35
==========================================
+ Hits 691 718 +27
- Misses 669 673 +4
- Partials 139 143 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@otoolep Prior to v3.36.0, We will either need a Go build tag to enable these new methods, or a tag to disable them. Personally, my preference would be to make it opt out so things "just work" by default, but technically that could be a breaking change for anyone currently using the |
Do you mean that the functions return like ErrNotImplemented? |
@mattn Historically this library has been inconsistent between having alternate version of the Go methods that return errors, or simply not defining the Go methods at all. I don't care which one we do here. The important part is that if you opt out (or don't opt in, depending on our approach), we never attempt to reference the C functions. |
I can add a build tag for either option (opt-in or opt-out), I forgot about the |
We can do check whether the SQLiteConn has methods like above, I think. @rittneje what do you think? |
@mattn The underlying problem is we cannot reference the
Us dynamically checking what methods |
Thanks your explaining. As you mentioned above, let's add |
To be clear, you are saying to do option 1 from my list? |
I have thought about this for a little while and I expect the following.
|
I assume that people who use libsqlite3 are expecting minimalism and they does not want these functions. I guess. |
Depends on their use case. They could also just want consistency across applications written in multiple languages. |
Merge upstream
Seems that #1085 changed the output of the sample program, but did not change the expectation in the GitHub action, so it fails. go-sqlite3/.github/workflows/docker.yaml Line 22 in 7476442
That said, I don't know why the output is checked in both the Dockerfile itself as well as the GitHub action. @mattn Can one of them be removed? |
I think we're good now. Let me know there are more required changes. |
What do you guys think, ready to merge? |
I think we're good to merge this? |
@rittneje -- I think we're good to merge this? Anything else need doing? The test has been updated, and all tests pass. With this merged I can return to using this repo, and dump my fork. |
Thank you! Looking forward to moving back to this repo, and away from my fork. |
@otoolep Thank you. |
Add support for Serialize and Deserialize, which wrap sqlite3_serialize and sqlite3_deserialize.
No description provided.