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

Ocaml adapters support for bucklescript #153

Merged
merged 4 commits into from
Oct 22, 2018
Merged

Conversation

rauanmayemir
Copy link
Contributor

I started working on adapters support for bucklescript and made a little progress on decoding variants. (I haven't yet committed test cases before getting some initial feedback, but I played around with it on a real project and was able to decode fairly convoluted variant type)

Is there a reason to have separate json_sum_adapter and json_record_adapter fields? When are they used at the same time?

P.S: I tried to follow common indentation/formatting, but it looks arbitrary. Do you use ocamlformat?

@mjambon mjambon requested a review from rgrinberg September 7, 2018 20:08
@mjambon
Copy link
Collaborator

mjambon commented Sep 7, 2018

Is there a reason to have separate json_sum_adapter and json_record_adapter fields? When are they used at the same time?

Good question. The short answers are "it's an internal design issue that requires some extra thinking" and "they're not used at the same time; they're the same thing, duplicated".

Right now, each kind of node of the AST has its own set of options. For example, it makes sense for a variant type to support the option <ocaml repr="classic"> which means "use classic variants instead of polymorphic variants". However, we don't have a good way to express that a given annotation applies to any kind of type. The special wrap construct kind of serves this purpose but it comes with its own meaning and I'm not sure it's the right solution here. So what I did is dodge the issue and decided that json adapters would be supported only for records and for variants, until we find the time to work on a better solution.

There are clearly use cases for json adapters outside of variants and records so this is on the roadmap. Obvious examples include simple types that don't follow json idioms (e.g. converting "true" to true) and unwrapped singletons (e.g. converting "items": 42 to "items": [42]). The corresponding task is #130.

For the moment, I'd suggest you proceed with a similar technique and support json adapters only for records and for variants.

@mjambon
Copy link
Collaborator

mjambon commented Sep 7, 2018

P.S: I tried to follow common indentation/formatting, but it looks arbitrary. Do you use ocamlformat?

No, I don't like the results produced by ocamlformat. I use ocp-indent to normalize indentation, that's it. We can set up a pre-commit hook to run ocp-indent and check line length automatically, see #110

@rauanmayemir rauanmayemir changed the title [WIP] Ocaml adapters support for bucklescript Ocaml adapters support for bucklescript Sep 28, 2018
@rauanmayemir
Copy link
Contributor Author

Just added tests for adapters, @rgrinberg I think this is ready for review.

I'd like to limit the scope of this PR to sum type only, as I'm not sure how record adapters supposed to work.

@rauanmayemir
Copy link
Contributor Author

A nudge to remind about this PR. @mjambon @rgrinberg

@rgrinberg
Copy link
Collaborator

Yeah, sorry about that. Let me give this an initial review right now.

@rgrinberg
Copy link
Collaborator

I merged the PR with a couple of minor cleanups and analogous record adapter support. Ignoring the field entirely for records isn't right, and there's no need to error out if we can provide simple support.

One thing that's a bit worrying is that the interface of both runtimes is differing. That's an issue as we would like to them to converge to a single interface with multiple implementations at some point.

@rgrinberg rgrinberg merged commit 364e9b6 into ahrefs:master Oct 22, 2018
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Dec 4, 2019
…nd atdgen (2.1.0)

CHANGES:

* Fix bug preventing generated code from compiling when using
  json adapters on recursive types.

* Improve automatic error messages shown in case of failed validation.
  Now include the validator's name or code.

* Add support for json adapters in the bucklescript backend. (ahrefs/atd#153)
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