-
Notifications
You must be signed in to change notification settings - Fork 53
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
Adapt to use Yojson 2.0 API #299
Conversation
We're gonna improve the pretty-printing a bit in Yojson 2.0.1: ocaml-community/yojson#141 |
Thanks for that work @Leonidas-from-XIV. I think it's best to wait for yojson 2.0.1 before merging this. The closest we're to the original output, the better. |
@mjambon I completely agree, I'll rebase this PR when we have 2.0.1 in place. Currently wrestling with getting 2.0.0 into OPAM without too much breakage but from there on, 2.0.1 should be smooth sailing. |
It uses `Seq` instead of `Stream` so the compatibility package can be dropped. Likewise it uses `Buffer` instead of biniou, so the generator code needs to be adapted.
60413b1
to
faac29e
Compare
Updated to use 2.0.1 which prettyprints the same way as the older release so no changes necessary, removed that commit altogether. |
Thank you for the update. This looks good to me. I'm on vacation without a
laptop for another 10 days so we can merge it but someone else would have
to make the release or any unexpected fixes.
…On Thu, Jul 7, 2022, 14:48 Marek Kubica ***@***.***> wrote:
Updated to use 2.0.1 which prettyprints the same way as the older release
so no changes necessary, removed that commit altogether.
—
Reply to this email directly, view it on GitHub
<#299 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACTZYIDHKAGCQJ2WHT45O3VS3GZDANCNFSM5XYVPWIQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@Leonidas-from-XIV @mjambon Thank you for working on this. We just hit this issue and are glad you have already fixed it. 👍 |
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.
Great, thanks again.
I didn't notice that |
This PR was resurrected as #311 |
Adapt to use Yojson 2.0 API (retry #299)
…n-codec-runtime and atd (2.10.0) CHANGES: * atdgen: use Yojson 2.0 API (ahrefs/atd#299) * atdpy: Support recursive definitions (ahrefs/atd#315) * atdts: fix nullable object field writer (ahrefs/atd#312)
Yojson 2.0 removed the
Stream
API to be forward-compatible with OCaml 5 (and stop triggering deprecation warnings in OCaml 4.14). To make atd use Yojson 2.0 going forward this PR modifies the generated code + the runtime to use the new functions. It comes with the small advantage of not needing to use theStream
-compatibility package anymore.Older versions have been restricted to use Yojson < 2.0: ocaml/opam-repository#21464
PR checklist
CHANGES.md
is up-to-date