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

Added tryExecuteStep and tryReset #142

Merged
merged 2 commits into from
Aug 28, 2017
Merged

Added tryExecuteStep and tryReset #142

merged 2 commits into from
Aug 28, 2017

Conversation

kaptenhonek
Copy link

fixes #121

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.04%) to 93.277% when pulling 8191046 on kaptenhonek:try_execute into 1a46a94 on SRombauts:master.

@SRombauts
Copy link
Owner

Great @kaptenhonek, thanks for your work on this!

I'll have a look at your code, but could you provide a basic unit test so that the code coverage does not decrease?

Cheers!

@kaptenhonek
Copy link
Author

Sure!

@kaptenhonek
Copy link
Author

Added tests (basically copied and adapted the test for executeStatement)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 96.429% when pulling f4947e7 on kaptenhonek:try_execute into 1a46a94 on SRombauts:master.

SRombauts added a commit that referenced this pull request Aug 28, 2017
@SRombauts
Copy link
Owner

Hi @kaptenhonek, thanks again. I've added two commits on (my copy of) your branch (see references above).

It mainly uses mbDone to return a SQLITE_MISUSE as was done previously with exceptions.

If it's okay for you, I'll merge it as is.

@kaptenhonek
Copy link
Author

Yeah, my initial commits was done quickly just to get things working, thanks for correcting the code :)
Should I close this merge request? (after you merge your changes)

@SRombauts SRombauts merged commit f4947e7 into SRombauts:master Aug 28, 2017
@SRombauts
Copy link
Owner

You did perfectly find!

I had to review the code, because it was getting too big/ugly, but this is just normal process.

Thanks you again!

@SRombauts
Copy link
Owner

Also, the merge-request has been closed automatically by the merge commit I pushed, AND the issue was closed since you referenced it in your first comment :)

@kaptenhonek
Copy link
Author

Awesome, thanks and good job :)

@kaptenhonek kaptenhonek deleted the try_execute branch August 29, 2017 09: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.

Statement executeStep and reset with return code instead of throwing?
4 participants