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

[RFD] adding DynamoDB #97

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

jrochel
Copy link

@jrochel jrochel commented Feb 24, 2021

Hello. I'd like to add DynamoDB support to this package.
However the code generated from the botocore specifications has the following
two issues:

  • It creates a module called Query. The generated code confuses the generated Query module with the Aws.Query since it opens Aws.
  • The specification contains mutual recursion, leading to mutually recursive modules.

Now, it's not too difficult to resolve these two issues by hand, after having generated the code, but I'm guessing that this is not the approach chosen for aws, so I'm wondering how to deal with these two issues properly.

  • The first issue is rather simple: Do not use open everywhere in the code generation.
  • The second issue is more tricky: Should there be a detection of mutual recursion? Or should there be a mutual recursion by default? In both cases one would need to generate module signatures as well. Is there another workaround?

I'm curious about your opinion!

Thanks,
Jan

@jrochel
Copy link
Author

jrochel commented Feb 24, 2021

(see also: #96)

@tmcgilchrist

@jrochel jrochel marked this pull request as draft February 24, 2021 12:45
@tmcgilchrist
Copy link
Collaborator

The second issue is more tricky: Should there be a detection of mutual recursion? Or should there be a mutual recursion by default? In both cases one would need to generate module signatures as well. Is there another workaround?

A number of services run into needing this mutual recursion, ideally I would like to detect the mutual recursion and do customised codegen for those cases. I believe making the whole types.ml module as one large mutual recursion definition will run into problems with the compiler having a limit on the size of such constructs.
I spoke to some people at ICFP a few years ago and they mentioned running into this problem, I haven't verified if it is an issue myself.

A really nasty short term fix would be to make the manual change as a patch, which gets applied after running code generation make target. I'm guilty of doing this locally when testing changes to the code generator.

The first issue is rather simple: Do not use open everywhere in the code generation.

I haven't tried this yet, I'd be interested to see the results.
@jrochel

@jrochel
Copy link
Author

jrochel commented Mar 4, 2021

As an alternative to patching, here's another way to organise the code: in branches.
One could have a branch for each service that needs manual adjustments. That branch would add to the master branch one commit for generating the code and another bunch of commits that fix mutual recursion and the like.

@tmcgilchrist
Copy link
Collaborator

Indeed, I'd see both patch and branch approaches as temporary fixes until the code generation could be made to deal with the issue. Could I suggest going with either one and then working on adding tests to verify the generated code works correctly?

@jrochel jrochel force-pushed the dynamodb branch 2 times, most recently from 6108ae2 to 4a661a9 Compare April 27, 2021 17:28
@Nymphium
Copy link
Contributor

I tried to use this PR branch with dynamodb-local and got an SSL error. How about switching http or https in Endpoints.url_of?

@Nymphium
Copy link
Contributor

After saying it I found out only Endpoints.endpoint_of "dynamodb" "local" returns "localhost" (and maybe it is for dynamodb-local). But dynamodb-local only accepts http request, not https. So I opened #121 for it.

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.

3 participants