Skip to content

Conversation

Leonidas-from-XIV
Copy link
Contributor

@Leonidas-from-XIV Leonidas-from-XIV commented Jun 3, 2022

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 the Stream-compatibility package anymore.

Older versions have been restricted to use Yojson < 2.0: ocaml/opam-repository#21464

PR checklist

  • New code has tests to catch future regressions
  • Documentation is up-to-date
  • CHANGES.md is up-to-date

@Leonidas-from-XIV
Copy link
Contributor Author

We're gonna improve the pretty-printing a bit in Yojson 2.0.1: ocaml-community/yojson#141

@mjambon
Copy link
Collaborator

mjambon commented Jun 6, 2022

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.

@Leonidas-from-XIV
Copy link
Contributor Author

@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.
@Leonidas-from-XIV
Copy link
Contributor Author

Updated to use 2.0.1 which prettyprints the same way as the older release so no changes necessary, removed that commit altogether.

@mjambon
Copy link
Collaborator

mjambon commented Jul 8, 2022 via email

@agarwal
Copy link

agarwal commented Jul 20, 2022

@Leonidas-from-XIV @mjambon Thank you for working on this. We just hit this issue and are glad you have already fixed it. 👍

Copy link
Collaborator

@mjambon mjambon left a comment

Choose a reason for hiding this comment

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

Great, thanks again.

@mjambon mjambon merged commit 87ec5c7 into ahrefs:master Jul 20, 2022
@mjambon
Copy link
Collaborator

mjambon commented Jul 21, 2022

I didn't notice that make test wasn't working before merging. I'm going to do something to revert this merge until we have a new yojson release that includes the fix ocaml-community/yojson#150

@mjambon
Copy link
Collaborator

mjambon commented Jul 21, 2022

This PR was resurrected as #311

mjambon added a commit that referenced this pull request Aug 10, 2022
Adapt to use Yojson 2.0 API (retry #299)
mjambon added a commit to mjambon/opam-repository that referenced this pull request Aug 10, 2022
…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)
@Leonidas-from-XIV Leonidas-from-XIV deleted the yojson-2-api branch September 8, 2022 08:50
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