-
Notifications
You must be signed in to change notification settings - Fork 49
Support alternative representations of variants #155
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
base: master
Are you sure you want to change the base?
Conversation
f0ee0a6 to
529e0f9
Compare
gasche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is impressive work and also a lot of code to review. Are you still interested in this? If yes, I could try to have a look.
README.md
Outdated
| | `\| RA` | `RA` | `["RA"]` | `"RA"` | `{"type": "RA"}` | `{"tag": "RA"}` | `<"RA">` | | ||
| | `\| RB of int` | `RB 42` | `["RB", 42]` | `{"RB": 42}` | (not supported) | `{"tag": "RB", "contents": 42}` | `<"RB":42>` | | ||
| | `\| RC of int * string` | `RC (42, "foo")` | `["RC", 42, "foo"]` | `{"RC": [42, "foo"]}` | (not supported) | `{"tag": "RC", "contents": [42, "foo"]}` | `<"RC":[42, "foo"]>` | | ||
| | `\| RD of { z : string }` | `RD {z = "foo"}` | `["RD", {"z": "foo"}]` | `{"RD": {"z": "foo"}}` | `{"type": "RD", "z": "foo"}` | `{"tag": "RD", "contents": {"z": "foo"}}` | `<"RD":{"z": "foo"}>` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table of examples is nice, but I would like to also have a list of all supported choices, with a description of the layout in English prose. (Before or after.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
Yes, I would like to finish this, to get better interoperability with other systems. |
|
Rebased. |
| 1. ``variants = `Array`` (default): variants are encoded as an array, where the first element is the constructor name and the subsequent elements are the constructor arguments. | ||
| 2. ``variants = `External``: variants are encoded as just the tag for empty constructors and as an object with a single field named after the constructor name and with the constructor arguments as its value otherwise. | ||
| 3. ``variants = `Internal "type"``: variants are encoded as an object containing a field named `"type"` with the constructor name as its value, as well as the fields of the inline record, if present; tuple constructors are not supported. | ||
| 4. ``variants = `Adjacent ("tag", "contents")``: variants are encoded as an object containing a field named `"tag"` with the constructor name as its value, and a field named `"contents"` with the constructor arguments as its value, if present. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updated documentation. If I understand correctly, you took care to implement those options for interoperability with the representation of variants in other libraries (serde, aeson, etc.). Which of those representations are interoperable with which libraries? Maybe it would be useful to mention it in the documentation here.
The only current representation has problems with readability and interoperability with JSON libraries for other languages, such as:
Closes #27, #32.