search

elixir-sqlite / sqlitex

An Elixir wrapper around esqlite. Allows access to sqlite3 databases.
https://hex.pm/packages/sqlitex
120 stars 34 forks source link

Allow esqlite’s timeout to be specified #65

Closed marramgrass closed 6 years ago

marramgrass commented 6 years ago

Hello.

First, thank you for the library. It's come in very useful for us.

We've been doing some work where we needed to increase the timeout esqlite applies to various operations. This pull request updates Sqlitex to expose the timeout argument in the various places that esqlite does so.

With these changes, Sqlitex defaults to the esqlite timeout of 5000 ms, and uses a combination of default values on the function arguments and implementing an Application-level config to leave the interface of Sqlitex unchanged for those who don't need to modify the timeout.

I think this is a broadly useful enough change to consider bringing into the upstream, but I worry that the implementation is a little noisy. Please let me know any changes you want made.

Thanks again.

sourcelevel-bot[bot] commented 6 years ago

Hello, @marramgrass! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

mmmries commented 6 years ago

@marramgrass thank you for this PR. I think that allowing the timeout to be passed through is a great idea and doing it with optional keyword options, then an Application config and finally a hardcoded default is a great idea. Thanks for your work on this.

marramgrass commented 6 years ago

For info, I will be coming back to this to update. Just very busy with other things at the moment. Sorry for the delay.

mmmries commented 6 years ago

@marramgrass thanks for letting us know. I'm happy to wait for you to make the changes, or if you'd like some help I can take some time to make those changes myself.

marramgrass commented 6 years ago

I've pushed a further commit making the discussed changes.

Please let me know if you'd rather I squashed them down to one.

mmmries commented 6 years ago

Thanks @marramgrass for this contribution. I think I'll change the name of the global config to match the db_timeout name. I'll try to get a release out today.

mmmries commented 6 years ago

Got 1.4.0 released with your changes included. Thank you again!

marramgrass commented 6 years ago

Thank you @mmmries and @obmarg for making my first contribution to the project such a pleasant and smooth experience 👍