Skip to content
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

Merged
merged 3 commits into from
Jul 20, 2022
Merged

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