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

Adding hook to vtable to allow VTab's to be updated #399

Merged
merged 1 commit into from Apr 5, 2017
Merged

Adding hook to vtable to allow VTab's to be updated #399

merged 1 commit into from Apr 5, 2017

Conversation

kenshaw
Copy link
Contributor

@kenshaw kenshaw commented Mar 24, 2017

This commit changes the vtable 'xUpdate' goModule field to a new
'cXUpdate' callback function which in turns calls a 'goVUpdate'
callback.

This new callback allows Go defined virtual table implementations
satisfying the VTabUpdater interface (also newly defined) a way to
delete/insert/update rows in a VTab.

Additionally, an anonymous interface is used within the goVUpdate
callback looking for 'TableName() string' which, when defined on a VTab
is used to provide a better contextual error message to end users if the
VTab is read only.

Care was taken to follow existing code style/conventions for this
addition, and for backwards-compatibility with existing VTab
implementations (hence why a new interface was required).

This commit changes the vtable 'xUpdate' goModule field to a new
'cXUpdate' callback function which in turns calls a 'goVUpdate'
callback.

This new callback allows Go defined virtual table implementations
satisfying the VTabUpdater interface (also newly defined) a way to
delete/insert/update rows in a VTab.

Additionally, an anonymous interface is used within the goVUpdate
callback looking for 'TableName() string' which, when defined on a VTab
is used to provide a better contextual error message to end users if the
VTab is read only.

Care was taken to follow existing code style/conventions for this
addition, and for backwards-compatibility with existing VTab
implementations (hence why a new interface was required).
@kenshaw
Copy link
Contributor Author

kenshaw commented Mar 24, 2017

I have posted this as a WIP, as I realize that there is likely more work that would need to be done to be accepted, such as unit tests, etc. However, I have gone ahead and put this up in the hopes of determining if this would be accepted in its general design, to see if the API design is satisfactory/unsatisfactory, and if so, to start discussion on what, if any API/interface would be acceptable instead.

Appreciate the consideration. Thanks in advance!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 62.632% when pulling b8936b7 on kenshaw:add-vtable-updater-hook into b2e4645 on mattn:master.

@kenshaw
Copy link
Contributor Author

kenshaw commented Apr 5, 2017

Any feedback on this PR? Let me know if this (or some kind of alternate form) would be a viable addition! Would love to get this in, somehow, into the main code base.

For reference, I'm working on a separate Go-based CSV driver, that uses this amazing package, and would like it to be able to "update" and "insert" rows. That can't be done without the update hooks.

Similarly, I had the idea to do the same for Google Sheets. I know it sounds silly, but I just get these ideas in my head and would love to be able to get them down. I'm more than happy to pursue these separately, but I really believe the update functionality directly in this package makes the most sense.

@mattn
Copy link
Owner

mattn commented Apr 5, 2017

Sorry my delay, I'll look into it in later.

@mattn
Copy link
Owner

mattn commented Apr 5, 2017

LGTM

@mattn mattn merged commit f68bb95 into mattn:master Apr 5, 2017
@mattn
Copy link
Owner

mattn commented Apr 5, 2017

Ah, test not found. could you please add test?

@kenshaw
Copy link
Contributor Author

kenshaw commented Apr 5, 2017

Great, thanks, appreciate it! Yes, I'll add a test immediately. I originally had just posted this in order to get feedback if the approach would be accepted or not, as the PR introduced a new type.

@kenshaw
Copy link
Contributor Author

kenshaw commented Apr 5, 2017

Sorry for the delay, I should have this done today.

@mattn
Copy link
Owner

mattn commented Apr 5, 2017

NP. I don't hurry. Feel free to do it. :)

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.

None yet

3 participants