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

Port to ppxlib registration and attributes #149

Merged
merged 13 commits into from
Apr 24, 2024

Conversation

sim642
Copy link
Contributor

@sim642 sim642 commented Jul 19, 2022

Analogous to ocaml-ppx/ppx_deriving#263. This doesn't necessarily depend on that PR, but some of the same criticism applies.

In particular the point about [%derive.to_yojson: ...] and [%derive.of_yojson: ...] being unsupported through ppxlib directly. According to sherlocode, only ocurrent uses one of these.
Nevermind, the support can be preserved using a custom extension.

@kit-ty-kate kit-ty-kate requested a review from pitag-ha July 19, 2022 17:00
@sim642
Copy link
Contributor Author

sim642 commented Apr 15, 2024

@NathanReb Since this is along the lines of ocaml-ppx/ppx_deriving#263 and that a release of ppx_deriving_yojson might be due (ocaml/opam-repository#25675 (comment)), then this would also make sense to synchronize similar changes.

@NathanReb
Copy link
Collaborator

Yeah, I was looking into this just yesterday and was pleasantly suprised to see you had already worked on a ppxlib port.

I will review this ASAP!

Copy link
Collaborator

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

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

It looks good! Thanks for working on this!

I have a few minor comments but besides that it should be good to go!

src/ppx_deriving_yojson.ml Outdated Show resolved Hide resolved
src/ppx_deriving_yojson.ml Show resolved Hide resolved
src/ppx_deriving_yojson.ml Outdated Show resolved Hide resolved
Copy link
Collaborator

@NathanReb NathanReb 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, thanks!

@NathanReb NathanReb merged commit a0e739d into ocaml-ppx:master Apr 24, 2024
1 check passed
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.

2 participants