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

[feature request] allow adapters in more locations #130

Open
ygrek opened this issue Jun 15, 2018 · 5 comments
Open

[feature request] allow adapters in more locations #130

ygrek opened this issue Jun 15, 2018 · 5 comments
Labels
backlog Low-priority task; likely to not happen feature request Big and small feature requests open problem We don't know a good way of addressing the problem target:ocaml Issues related to the OCaml backend (atdgen)

Comments

@ygrek
Copy link
Contributor

ygrek commented Jun 15, 2018

Usecase:
Some api returns a field in record sometimes as string and sometimes as string list. This can be currently hacked around with json wrap but looks like adapter could provide a natural solution when applied on record field position.

@mjambon
Copy link
Collaborator

mjambon commented Jun 17, 2018

Thanks for being an early adopter of the feature!

Yes, it would be nice to support json adapters everywhere. Converting strings to numbers or booleans seem like a common issue.

I wasn't sure how to implement this in an elegant way so I left it for later. The issue is only to avoid code duplication in atdgen. Perhaps we can piggy-back on the implementation of wrap constructors (since they can be applied to any node). I don't know. It needs work.

@mjambon mjambon added feature request Big and small feature requests backlog Low-priority task; likely to not happen open problem We don't know a good way of addressing the problem labels Jun 17, 2018
@gtrak
Copy link

gtrak commented Jul 2, 2018

I could use this on plain lists, was confused that it didn't work given the documentation

@mjambon
Copy link
Collaborator

mjambon commented Jul 2, 2018

@gtrak I agree with the need to support the feature on lists. I actually thought we did so it's possible that I wrote that somewhere (in an announcement maybe?). The reference manual is correct, though (https://atd.readthedocs.io/en/latest/atdgen.html#field-adapter-ocaml).

@gtrak
Copy link

gtrak commented Jul 2, 2018

I wasn't able to make it work, then traced it to the source and the emitter seems specific to record and variant types? If we don't yet support it, it might be better to syntax error than silently fail? I might take a look at this eventually since my workaround was really really ugly.

@mjambon
Copy link
Collaborator

mjambon commented Jul 2, 2018

@gtrak the technical issue is to avoid code duplication. Right now the <json adapter.ocaml=...> annotation is translated separately for each kind of node of the AST. In oj_emit.ml, we have:

      let adapter = j.json_sum_adapter in
      write_with_adapter adapter standard_writer

and for records we have:

      let adapter = j.json_record_adapter in
      write_with_adapter adapter standard_writer

Having wrap nodes in the core atd syntax is one way we solved this problem in the past, but I don't think they're suitable for use with json adapters. I also hope we don't have to create yet another kind of artificial node in the ast.

@mjambon mjambon added the target:ocaml Issues related to the OCaml backend (atdgen) label Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Low-priority task; likely to not happen feature request Big and small feature requests open problem We don't know a good way of addressing the problem target:ocaml Issues related to the OCaml backend (atdgen)
Projects
None yet
Development

No branches or pull requests

3 participants