Add support for tokio-rusqlite

[czocher]

@cljoly please have a look and comment everything that you feel is not right. Consider it a very preliminary version.

There's a few things that are still missing:

• Tests are currently just copied over from the sync version to the async version. Feels more like doctests should be considered for both versions and the main tests should be performed only on the extracted functions?
• The created feature is not additive. I'm not sure how to make it so, maybe providing a AsyncMigrations struct and leaving the Migrations without any changes (except extracting the functions) may be a solution? What do you thing @cljoly?
• Additional tests can and should be created for the extracted functions in lib.rs.

Closes #24.

[czocher]

Ah I see you merged the PR to add the up/down hooks. I'll rebase it on the current master later ;).

[czocher]

@cljoly I had a far better idea how this could be implemented so that:

1. The changes to the base project are minimized.
2. The changes are additive.

Have a look at: https://github.com/Czocher/rusqlite_migration/pull/new/support-tokio-sqlite-v2

What I did in v2 is to create a simple AsyncAdapter holding the base Migrations struct in an Arc. For an example of the usage of it you can have a look at: https://github.com/Czocher/rusqlite_migration/blob/support-tokio-sqlite-v2/examples/quick_start_async.rs

Tell me which version you like best (IMHO v2 better handles your requirements as to the implementation).

The v2 branch should also be considered a work in progress:

• needs cleanup
• can move some stuff to a separate module
• requires unit tests
• we should discuss if Migrations are ever used with a lifetime 'm which is not 'static. If Migrations are always 'static it would simplify the async version slightly.

[cljoly]

@czocher thanks for opening this PR, I'll try and take a look on Sunday.

[cljoly]

Sorry I am very busy this week, I did not get the chance to look into your PR, hopefully this weekend.

[czocher]

@cljoly there's no rush :). I suggest you first look into what's proposed in this comment: #29 (comment)

[cljoly]

I took a look at your v2, it’s much better in terms of code duplication, I quite like it actually! Perhaps you could push the commits from the v2 branch into this branch, to ease further review?

Just a minor point, we could rename AsyncAdapter to AsyncMigration there.

[cljoly]

Tests are currently just copied over from the sync version to the async version. Feels more like doctests should be considered for both versions and the main tests should be performed only on the extracted functions?

Yes, but this is much less of a problem in your v2 branch :)

Additional tests can and should be created for the extracted functions in lib.rs.

We want to have good test coverage indeed 👍🏼

[cljoly]

we should discuss if Migrations are ever used with a lifetime 'm which is not 'static. If Migrations are always 'static it would simplify the async version slightly.

Right, I think there are cases where you don’t use 'static. In any case, I believe going to a more restrictive lifetime parameter would be a breaking API change, so that should be avoided if possible.

[cljoly]

I appreciate all the work you have put in this @czocher and sorry I could not review earlier. Hopefully, I have addressed all your questions (that were not solved in v2).

[cljoly]

Ah I see you merged the PR to add the up/down hooks. I'll rebase it on the current master later ;).

That’s fine and I suspect you will have less conflicts (if at all in the v2 branch)

[czocher]

Replaced the branch in this PR with the the v2 branch. I implemented is the rename from AsyncAdapter to AsyncMigrations.

What needs to be done before this change should be reviewed:

• Extract the AsyncMigrations to an asynch submodule.
• Add documentation.
• Add unit tests.
• Update the README.md to indicate that tokio-rusqlite is supported and point to the example.
• Fix the quick_start_async.rs to be run only when async-tokio-rusqlite is enabled. This may require a change of structure similar to how other packages (like axum-login) do it i.e. a workspace with subprojects. Can you make this change @cljoly or do you want me to provide a PR?

What kind of unit tests should be provided @cljoly?

[cljoly]

I’ll look more into this later, but here are some first few comments.

[cljoly]

I need to think a bit more about unit tests and how to disable the async example by default.

And also,

• the CI would need to be changed to test with and without the feature (I can take care of it if you want)

[czocher]

I need to think a bit more about unit tests and how to disable the async example by default.

From what I saw usually people make a cargo workspace in the main package directory and then make the examples separate projects which include the features they require.

the CI would need to be changed to test with and without the feature (I can take care of it if you want)

Check if I did it correctly ;). I added a test with all-features to future-proof against non-additive features which may be happen at some point.

[cljoly]

From what I saw usually people make a cargo workspace in the main package directory and then make the examples separate projects which include the features they require.

Since I have not found a better option, let’s do that if you don’t mind.

Check if I did it correctly ;). I added a test with all-features to future-proof against non-additive features which may be happen at some point.

The CI looks great, thanks!

[czocher]

Since I have not found a better option, let’s do that if you don’t mind.

I implemented the structure change here since it's required for this PR to compile. If you feel like it should be extracted to a separate PR let me now ;).

I still have a problem with changing the working-directory for the sync-readme task, maybe you have an idea how to run it from inside rusqlite_migration?

EDIT: I think I managed to fix sync-readme.

[czocher]

In my opinion this task is mergeable at the moment, only thing potentially missing are unit tests for the async version. I wrote an async_integration_test to do some basic checks.

[cljoly]

In my opinion this task is mergeable at the moment

Great! I should be able to take a deeper look this weekend and if everything goes well, to merge.

[cljoly]

I think it looks great, and this can be further refined if needed in the future. I’m sorry I introduced a conflict when merging another PR, I’ll solve the conflict when I get the chance and merge.

[cljoly]

Alright, I’ve also fixed a markdown link :)

[cljoly]

Thank you so much for your contribution and your patience!