search

janestreet / sexplib

Automated S-expression conversion
MIT License
147 stars 27 forks source link

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

Open gasche opened 2 years ago

gasche commented 2 years ago

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 commented 2 years ago

I also reported this as a Csexp issue: https://github.com/ocaml-dune/csexp/issues/19 .

gasche commented 2 years ago

Over at https://github.com/ocaml-dune/csexp/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

https://github.com/ocaml-dune/csexp/issues/19#issuecomment-1187633459

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 commented 2 years ago

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