Add support for both Async and Lwt

roddyyaga / ppx_rapper

Syntax extension for writing SQL in OCaml

Other

139 stars 18 forks source link

Add support for both Async and Lwt #10

Closed bikallem closed 3 years ago

bikallem commented 3 years ago

Summary of the PR, fixes #10

Generalise ppx to support both async and lwt
Adds two new packages ppx_rapper_async and ppx_rapper_lwt for async and lwt respectively
Adds a functor Rapper.Make_helper. This is the generalised functionality which enables rapper to support both lwt and async
Add examples/async for async usage example. Corollarily, the examples/lwt folder to hold lwt usage example.
Updated tests to reflect new code generation scheme, i.e. usage of Rapper_helper module.

anmonteiro commented 3 years ago

Using this PPX after this change is now more complicated, and it looks like there's a weird interaction between the PPX and the I/O runtime libs.

I am now required to open one of them before using the PPX, in order to bring some module in scope. IMO it looks like implicit magic and it's gonna make this very cumbersome to use.

bikallem commented 3 years ago

I am now required to open one of them before using the PPX, in order to bring some module in scope. IMO it looks like implicit magic and it's gonna make this very cumbersome to use.

@anmonteiro Thanks for the feedback. I have pushed a commit to address the cumbersome part. One doesn't need to do open Ppx_rapper_lwt or open Ppx_rapper_async to use ppx_rapper now. For the implicit part, before this PR lwt and caqti-lwt was always in implicit scope. Now, both those libs are in implicit scope only if they use ppx_rapper_lwt. Similarly, when using ppx_rapper_async, async and caqti-async are in implicit scope.

roddyyaga commented 3 years ago

Thanks, this looks great! I will try to merge and release later this week.

anmonteiro commented 3 years ago

Apologies for the late feedback -- I only got to try this now.

I'm glad we could avoid the problem of having to open the runtime library upon usage of the PPX. What still felt a little weird just now was having to add both ppx_rapper.runtime and ppx_rapper_lwt to the libraries section of my dune file. I feel like we could probably do better here. Any suggestions?

roddyyaga commented 3 years ago

@anmonteiro I agree. The lwt and async libraries depend on ppx_rapper.runtime so you should be able to just directly depend on ppx_rapper_async/lwt I think, as in the examples.