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

optional parameter for ppx_bin_prot #125

Open
pbiggar opened this issue Apr 9, 2020 · 6 comments
Open

optional parameter for ppx_bin_prot #125

pbiggar opened this issue Apr 9, 2020 · 6 comments

Comments

@pbiggar
Copy link

pbiggar commented Apr 9, 2020

ppx_deriving supports an optional flag. From their docs:

It's possible to make deriving ignore a missing plugin rather than raising an error by passing an optional = true option, for example, to enable conditional compilation:

type addr = string * int
[@@deriving yojson { optional = true }]

This works well for their builtins, but doesn't seem to work for bin_io - which is making writing platform independent code difficult. The bin_io folks suggested that this would need to be implemented here.

Is this the place to support optional for all ppx derivers?

@pbiggar pbiggar changed the title optional parameters for ppx_bin_prot optional parameter for ppx_bin_prot Apr 9, 2020
@ghost
Copy link

ghost commented Apr 14, 2020

What is the use case for this? I mean, how can a ppx rewriter be present or not?

@pbiggar
Copy link
Author

pbiggar commented Apr 14, 2020

What is the use case for this? I mean, how can a ppx rewriter be present or not?

My main use case is sharing code between our two apps, one in bucklescript/ReasonML, and the other using native OCaml/dune. Though we use several PPXes in bucklescript, however some we only use in native (ppx_deriving_yojson which already supports optional which made this easy to handle; and ppx_bin_prot)

@ghost
Copy link

ghost commented Apr 15, 2020

I see. That seems like a valid use case to me, thanks for explaining. That is indeed the right place to support optional for all ppx derivers. Do you want to give it a go?

@pbiggar
Copy link
Author

pbiggar commented Apr 15, 2020

Do you want to give it a go?

I can try. Can you give me a rough outline of what you'd want a solution or implementation to look like?

@ghost
Copy link

ghost commented Apr 16, 2020

Sure, so I don't have the code in mind right now and little time to dig in on this subject, so I will only give you high level pointers and will let you do a bit of research for now :)

Essentially, at some point we resolve the string "ppx_bin_prot" into a Deriver.t, and we fail if we cannot find it. That were you want to patch the code: if we can't find it, you should like if there is optional:true in the list of arguments. To find out where this code is, you can start by grepping for the error message and walk your way up to the lookup function.

@sim642
Copy link
Contributor

sim642 commented Jul 26, 2022

This is related to ocaml-ppx/ppx_deriving#247, which really is just this ppxlib issue. if ppxlib's deriving transformation handled the special optional argument as well, then both issues would be solved.

It would be entirely possible to do this in ppxlib, I just wonder whether this is the most appropriate way to support optional derivers. It kind of hijacks one argument from the derivers' declared argument space.
Maybe something separate like [@@deriving_optional ...] would be nicer? Of course that doesn't provide the seamless compatibility between ppx_deriving and ppxlib.

Or given the extremely rare (desired) use of optional derivers, maybe it's fine to not even provide anything for it out of the box, but just require it to be handled on the user's side. I think it would be fairly simple to define a custom in-project deriver that's conditionally just Ppxlib.Deriving.add_alias for the desired one (or not). I guess that would require #124 though.

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

No branches or pull requests

2 participants