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

makeVariables #35

Merged
merged 12 commits into from
Nov 23, 2019
Merged

makeVariables #35

merged 12 commits into from
Nov 23, 2019

Conversation

jfrolich
Copy link
Collaborator

@jfrolich jfrolich commented Nov 4, 2019

I never did anything with ppx but this seems to pass tests. Any recommendations how to proceed. I only implemented the bucklescript side of things for now.

Resolves #33

@baransu
Copy link
Collaborator

baransu commented Nov 4, 2019

I rebased it on top of the current master. GH Actions should work correctly right now.

Looks really good. Thank you! The native side should bs as straight forward as the BuckleScript part 🙂. After that, we can merge 😎

@jfrolich
Copy link
Collaborator Author

jfrolich commented Nov 6, 2019

@baransu do you know of a good guide to understand ppx and the ocaml/reason AST? Perhaps especially documentation about how to use the Ast_helper module (it's not really documented).

@baransu
Copy link
Collaborator

baransu commented Nov 8, 2019

@jfrolich In my case, I was learning as I go with graphql_ppx. I just follow https://caml.inria.fr/pub/docs/manual-ocaml/compilerlibref/Parsetree.html to know what to do basically and a lot of trial and error.

@jfrolich
Copy link
Collaborator Author

jfrolich commented Nov 8, 2019

Awesome. ReasonML docs already makes it more readable. But pretty sparse docs indeed!

@jfrolich
Copy link
Collaborator Author

I added the makeVariables to native and the tests seem to pass. I could only run @402 for some reason.

@baransu baransu self-assigned this Nov 22, 2019
@baransu
Copy link
Collaborator

baransu commented Nov 23, 2019

LGTM 🎉 Thank you @jfrolich

@baransu baransu merged commit 667673c into teamwalnut:master Nov 23, 2019
@nirvdrum
Copy link

nirvdrum commented Dec 1, 2019

@jfrolich When you get a chance, would you mind adding something to the README? I'm sure I can pull together the info by looking at the PR, but I think a short paragraph with a code example would go a long way in helping others out.

@baransu
Copy link
Collaborator

baransu commented Dec 4, 2019

@nirvdrum Created issue for that #55

@jfrolich
Copy link
Collaborator Author

jfrolich commented Dec 5, 2019

Good point @nirvdrum.

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.

Expose makeVariables and makeMutation
3 participants