Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

WIP: Add copy mode (take 2) #28

Merged
merged 5 commits into from
Jul 29, 2019
Merged

Conversation

leamingrad
Copy link
Contributor

This is a second attempt at implementing the copy mode function discussed in #26, but using the new populate method discussed in #27. I have created a new MR since this is quite different from the first attempt, and I am happy to either close that MR or just overwrite the branch as preffered.

Implementing in this way is definitely easier, but there are a couple of things I am unhappy with, specifically:

  • Due to the way Caqti_stream.iter_s is implemented, the error type for the output must match the error type of the input. This doesn't make a lot of sense in the inbound direction, as there are not likely to be any errors in the input stream. Is it worth creating a new iter method that allows the input and output error types to be the same?
  • I had a lot of trouble getting the compiler to accept the polymorphic variant behaviour for the error types - is there a better way I could handle this? I think it would probably be simpler if I had all the populate methods implemented at once (then I could drop the Not_implemented error), but then that would mean that other drivers would have to push towards full implementation straight away

@paurkedal
Copy link
Owner

I think this is a good approach. About the two issues:

  • The Caqti_stream module is yet unreleased, so we can generalize the error of both iter_s and fold_s before the release. It's possible to use subtyping like in Caqti_response_sig, due to the error variant being parametric; we would need row polymorphism for that, I think. So, the solution I can see is to could wrap the errors in another variant like

    val iter_s :
      f:('a -> (unit, 'errc) result future) ->
      ('a, 'err) t ->
      (unit, [`Self of 'err | `Callback of 'errc]) result future

    Any opinions on this (CC @NathanReb)?

  • I am not sure about the precise trouble, and if it's related to the above, but two things good to know when working with variants is that you can match sub-variants with the #variant_name pattern and, as you do already, cast with :>. That said, I think we can require drivers to implement populate. At least for SQL-based drivers it is possible to make a generic implementation.

@leamingrad
Copy link
Contributor Author

I've pushed a new set of commits with the following changes:

  • The errors for fold_s and iter_s are generalized as above
  • All drivers now implement the same basic version of populate that sqlite does
  • There is no not_implemented error

Would it be possible to get this PR reviewed and merged, then I will put in another PR to improve the implementation of populate for postgres?

@paurkedal
Copy link
Owner

Looks good, just add the ~oneshot:true arguments, then we can merge.

@paurkedal paurkedal merged commit 099577e into paurkedal:master Jul 29, 2019
@leamingrad leamingrad deleted the add_copy_mode_2 branch July 30, 2019 07:51
@leamingrad leamingrad mentioned this pull request Jul 30, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants