Skip to content

Add a feature to allow any json in a variant#55

Merged
anmonteiro merged 6 commits into
melange-community:mainfrom
EmileTrotignon:allow_any
Mar 7, 2025
Merged

Add a feature to allow any json in a variant#55
anmonteiro merged 6 commits into
melange-community:mainfrom
EmileTrotignon:allow_any

Conversation

@EmileTrotignon
Copy link
Copy Markdown
Collaborator

Syntax is as follow :

type t =
| Other of Yojson.Basic.t [@allow_any]
| Foo
[@@deriving json]

Then, if the json j is invalid for type t, Other j is returned instead.

There is support for both runtime and js (although their Json type is different: is that an issue ? We could introduce a [%json] extension that translates to Js.Json.t in Js and Yojson.Basic.t in runtime if its an issue.)

As a bonus, I improved an error message that slipped through in my error message PR. Because its only on JS I did not find it with my cram tests.

@EmileTrotignon
Copy link
Copy Markdown
Collaborator Author

EmileTrotignon commented Feb 7, 2025

I also moved the common folder inside the native one. The reason for that is that then I can use it with include subdirs, which enables merlin inside it, which is very useful for developing.

using copy_files means that merlin does not work inside these files.

@EmileTrotignon
Copy link
Copy Markdown
Collaborator Author

Something similar exists in atdgen, it only allows extra tags instead of any json. I think allowing any json is a good idea, especially because we have structured types for json so we can easily check afterward if its tag-shapped.

Copy link
Copy Markdown
Member

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense, no strong opinions

Comment thread ppx/browser/ppx_deriving_json_js.ml Outdated
Comment thread ppx/browser/ppx_deriving_json_js.ml Outdated
Comment thread ppx/native/common/ppx_deriving_tools.ml Outdated
Comment thread ppx/native/common/ppx_deriving_tools.ml Outdated
Comment thread ppx/native/common/ppx_deriving_tools.ml Outdated
Comment thread ppx/native/common/ppx_deriving_tools.ml Outdated
Comment thread ppx/native/common/ppx_deriving_tools.ml Outdated
Comment thread ppx/native/ppx_deriving_json_native.ml Outdated
@EmileTrotignon
Copy link
Copy Markdown
Collaborator Author

Hey @anmonteiro I have added the label you asked, and remove the stray comment. Any other change required ?

Copy link
Copy Markdown
Member

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@EmileTrotignon
Copy link
Copy Markdown
Collaborator Author

Could this be merged ? I don't have right to do so @anmonteiro

@anmonteiro
Copy link
Copy Markdown
Member

We should still snapshot the tests that are failing before merging

@EmileTrotignon
Copy link
Copy Markdown
Collaborator Author

Yeah. The failures are not reproduced on my machine, so I am not sure how to do it.

@andreypopp
Copy link
Copy Markdown
Collaborator

This seems to be the difference in ocamlformat versions installed on CI vs local.

@EmileTrotignon
Copy link
Copy Markdown
Collaborator Author

There should be a .ocamlformat file with a fixed version to avoid that.

@EmileTrotignon
Copy link
Copy Markdown
Collaborator Author

There is a test that is failing or succeeding depending on the version. I think this one is impossible to promote.

Should I try and fix or is this good enough ?

@anmonteiro
Copy link
Copy Markdown
Member

hey @EmileTrotignon, I just pushed a commit fixing this test so that we can promote it for all ocaml versions.

@EmileTrotignon
Copy link
Copy Markdown
Collaborator Author

This is ready for merging

@anmonteiro anmonteiro merged commit 4838a99 into melange-community:main Mar 7, 2025
jchavarri added a commit that referenced this pull request Mar 8, 2025
* main:
  Add a feature to allow any json in a variant (#55)
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

Successfully merging this pull request may close these issues.

3 participants