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

Csexp.t and Sexplib.Type.t are incompatible #44

Open
gasche opened this issue Jul 14, 2022 · 7 comments
Open

Csexp.t and Sexplib.Type.t are incompatible #44

gasche opened this issue Jul 14, 2022 · 7 comments
Labels
forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system.

Comments

@gasche
Copy link

gasche commented Jul 14, 2022

Csexp.t and Sexplib.Type.t are two identical but incompatible datatype definitions. This is inconvenient (see for example https://gitlab.inria.fr/bmontagu/sexp_decode/-/issues/1). Would it be possible to share the same type definition, by having one library depend on the other?

@gasche
Copy link
Author

gasche commented Jul 14, 2022

I also reported this as a Csexp issue: ocaml-dune/csexp#19 .

@github-iron github-iron added the forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system. label Jul 15, 2022
@gasche
Copy link
Author

gasche commented Jul 18, 2022

Over at ocaml-dune/csexp#19 we discussed the idea of having a sexplib-type package that would contain just the definition of a sexplib type, and be a dependency of both Sexplib and Csexp. @rgrinberg was asking whether the Sexplib people would agree with that approach.

@gasche
Copy link
Author

gasche commented Nov 17, 2022

Gentle ping. This looks like a rather simple change to me (simplest change: move the type definition to its own sexplib-type package), and I don't know how to get feedback. (I guess that @github-iron is not a human, and I'm not sure who to ping.)

@staronj, you made the last commit here and are probably currently the person in charge of "open-source releases" for many Jane Street packages, are you the right point of contact and/or would you know who to get in touch with if @github-iron is not effective enough at making people give feedback?

@snowleopard
Copy link

Pinging @github-iron did end up sending an email to my Inbox, so it's effective, at least for now :)

Sexplib is maintained by @ceastlund so it's up to him to make the call.

@ceastlund
Copy link
Member

sexplib0 was essentially intended to be that library: the sexp type, plus enough support for ppx_sexp_conv to do its job. Is there an objection to using it for this purpose?

@gasche
Copy link
Author

gasche commented Nov 18, 2022

I don't have any opinion on this myself, but I understood that @rgrinberg had potential objections on the Csexp.t side:

ocaml-dune/csexp#19 (comment)

Honestly, I'm a bit hesitant to depend on sexplib0 in csexp. I have no doubt it's a quality library, but portability is not as high of a concern for JST as it is for the packages that rely on csexp (dune, merlin, lsp).

@ceastlund
Copy link
Member

Okay, I don't object to a sexp_type library if that's the concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system.
Projects
None yet
Development

No branches or pull requests

4 participants