Port ppx from andreypopp/ppx_deriving #10

jchavarri · 2024-08-14T17:31:25Z

This PR ports @andreypopp's PPX to this repo.

~~For now, the current state is "dune build works" 😅 . But it's a good point to open discussion.~~ Edit: all issues have now been fixed.

Proposal

The idea is to offer the json PPX as part of the melange-json package. As an addition, native compatibility is offered as well through Yojson.

Having everything in a single repo / package makes maintenance easier for maintainers, and usage more straight forward for users.

(As a nice side effect, we got GH actions workflow for opam 💰 )

Questions

Should we modify the native ppx so that it works with a flag -native? I personally think it's cleaner to leave the current approach with its own ppx / exe
Are internal names for libraries ok? e.g. ppx_deriving_json_browser
Are browser and native suffixes (suffices?) ok?

TODO

Add changelog entry
Fix nix build
Fix 4.14 build

jchavarri · 2024-08-15T07:43:33Z

I renamed all appearances of browser to js in 460e4a4.

Another thing might be worth looking at (but can do later) is to trim Ppx_deriving_tools module, it has more code than needed for this ppx and more things exposed than needed for this ppx.

I tried cleaning this up but didn't see any obvious / easy things to remove. For now I just moved tools folder inside ppx to keep things tidy, as this folder is exclusively needed in the ppx.

I also updated the ppx readme to remove custom opam repo addition as it won't be needed anymore, added a changelog entry, and fixed the 4.14 compatibility.

The last remaining thing that I can think of is fixing CI for nix, @anmonteiro could you give me a hand with that please? 🥺

andreypopp · 2024-08-15T07:49:00Z

another thought on naming, maybe for ppx it's better to have ppx in its name?

melange-json.ppx for js ppx
melange-json.ppx-native for native ppx
melange-json.ppx-runtime for runtime (both js and native)

I quite often use ocamlfind list | rg ... to see what's available and having ppx in there will hint how to use the lib

jchavarri · 2024-08-15T07:51:37Z

Yes, I like that naming much more. It's also consistent with other ppxs in Melange ecosystem, like the pervasive melange.ppx.

jchavarri · 2024-08-15T07:57:26Z

@andreypopp I did the renaming in d3c6e5d.

melange-json.ppx-runtime for runtime (both js and native)

I wasn't sure how to use the same name for both (there can only exist one public lib name at the moment), so I went with melange-json.ppx-runtime for js, and melange-json.ppx-native-runtime for native.

jchavarri · 2024-08-15T07:58:32Z

and melange-json.ppx-native-runtime for native.

Actually melange-json.ppx-runtime-native after last commit 😓

andreypopp · 2024-08-15T08:27:54Z

ah, yeah, not possible to have clashes in public names, but not a big deal, these libs will be added by dune automatically once you use a corresponding ppx

anmonteiro · 2024-08-16T02:03:06Z

I'm also wondering what your preference is regarding the ppx being its own package, e.g.

melange-json for the melange library
melange-json-ppx for the ppx? and the library names could be:
- melange-json-ppx for the melange ppx
- melange-json-ppx.native for the native ppx

No strong preference here, just providing an alternative

anmonteiro · 2024-08-16T02:05:39Z

The last remaining thing that I can think of is fixing CI for nix, @anmonteiro could you give me a hand with that please? 🥺

I pushed a commit fixing this

andreypopp · 2024-08-16T04:33:17Z

few more points:

need to unify exceptions between melange-json and the ppx, I think this is important
let's leave the native part undocumented for now, I think we'd want to transition to Yojson.Safe.t before making it public

andreypopp · 2024-08-16T04:42:35Z

I'm also wondering what your preference is regarding the ppx being its own package

other options:

have js ppx in melange-json package but split away native ppx into a separate package
split js ppx and native ppx into separate packages (3 packages total counting melange-json)

the nice thing about splitting away native ppx is that we won't impose yojson dep on melange users and we won't impose melange dep on native users

jchavarri · 2024-08-16T07:37:58Z

need to unify exceptions between melange-json and the ppx, I think this is important

@andreypopp could you elaborate on how this unification would look like? melange-json has DecodeError and ParseError. the new ppx has Error, which is used exclusively for the "Not supported" messages. I don't see how they could be unified?

I'm also wondering what your preference is regarding the ppx being its own package

other options:
* have js ppx in melange-json package but split away native ppx into a separate package

* split js ppx and native ppx into separate packages (3 packages total counting melange-json)

I prefer the "single package, until proved otherwise" approach. Having a single package makes things easier for maintainers and users. The "until proved otherwise" means that if some user some day (or ourselves), comes with a legit use case where separate packages are needed, then we can always split. Otherwise, it seems like a preoptimization.

A couple of examples where multi package made things harder for us in the past (in this case, it's reason-react and reason-react-ppx):

As maintainers, publishing in opam might lead to cycles in dependencies between the packages, example
As users, I spent (just yesterday) 20min blocked on some issue with the code not updating, to later realize I was pinning reason-react, but not reason-react-ppx

Maybe I am missing what are the main upsides of having multiple packages? The "worst" part of single package is that as a user I am downloading and installing deps that I am not using, but all things considered it doesn't outweight the downsides imo.

#10 (comment)

jchavarri · 2024-08-16T07:40:46Z

let's leave the native part undocumented for now, I think we'd want to transition to Yojson.Safe.t before making it public

Done in 3a31039.

#10 (comment)

andreypopp · 2024-08-16T08:42:38Z

@andreypopp could you elaborate on how this unification would look like? melange-json has DecodeError and ParseError. the new ppx has Error, which is used exclusively for the "Not supported" messages. I don't see how they could be unified?

@jchavarri the ppx runtime uses this exception and melange-json has DecodeError, so I think ppx runtime should also use DecodeError — that way if users would want to catch the exception and recover — they will only have to do this for DecodeError.

andreypopp · 2024-08-16T08:47:41Z

let's leave the native part undocumented for now, I think we'd want to transition to Yojson.Safe.t before making it public

Done in 3a31039.

The README still mentions yojson/native. By the way, should we move docs from this README to the main project README, why have two?

jchavarri · 2024-08-16T09:40:45Z

the ppx runtime uses this exception and melange-json has DecodeError, so I think ppx runtime should also use DecodeError — that way if users would want to catch the exception and recover — they will only have to do this for DecodeError.

updated the ppx to use DecodeError in b02fb3c. pls lmk if this is what you had in mind.

The README still mentions yojson/native. By the way, should we move docs from this README to the main project README, why have two?

both should be fixed in 19fcb09.

andreypopp

All looks good to me, thank you!

CHANGES: - Port PPX from @andreypopp/ppx_deriving_json ([melange-community/melange-json#10](melange-community/melange-json#10))

andreypopp and others added 30 commits November 14, 2023 23:58

init

d6db77a

init

232bac5

update *.opam

ebe4238

chore: add .gitignore and add name on dune-project

15448cf

feat: pick add attributes changes

efbce08

feat: add attributes on repr type_decl

a6c3ec3

add derive of variant enum case

5beda6f

make derive_of_enum cases optional, defaults to non enum case

692aab6

add array and float codec, use js error on browser runtime

d2d8ed4

json.as, json.key, json.default, and enum variant and polyvar case

f60625e

update cram tests

dd78fd1

reorganize code

97883df

update for newer ppx_deriving_tools

8f48698

expose original ast as context in repr

231e97d

rework attribute code

e50a7b1

add docs

0c6657f

add docs

734204e

README

bbe7928

README

9ea406b

README

31c508a

README

1b09e98

.ocamlformat

bb5eaaa

README

ec465bc

README

f019e2a

.github/workflows: add main

fe68285

.github/workflows: tweak opam repo setup

ab9f243

generate boilerplate with dune

5908fdf

native: fix handling default record field value

f026bee

update tests

5fb4451

initial commit

2d89eba

rename ppxs and runtimes

d3c6e5d

better native naming

4afc0bc

jchavarri mentioned this pull request Aug 15, 2024

step 5: decoding data the easy way ahrefs/react-alicante-workshop#4

Merged

fix nix workflow

d95fd08

jchavarri added a commit that referenced this pull request Aug 16, 2024

ppx: undocument native ppx

7b7e792

#10 (comment)

ppx: undocument native ppx

3a31039

#10 (comment)

jchavarri force-pushed the ppx branch from 7b7e792 to 3a31039 Compare August 16, 2024 07:41

jchavarri added 2 commits August 16, 2024 09:30

ppx: use DecodeError

b02fb3c

ppx: move readme content to main one, remove yojson refs

19fcb09

andreypopp approved these changes Aug 16, 2024

View reviewed changes

jchavarri merged commit c615cb1 into main Aug 16, 2024
7 checks passed

jchavarri deleted the ppx branch August 16, 2024 10:16

jchavarri mentioned this pull request Aug 16, 2024

[new release] melange-json (1.2.0) ocaml/opam-repository#26366

Merged

jchavarri added a commit to jchavarri/opam-repository that referenced this pull request Aug 16, 2024

[new release] melange-json (1.2.0)

6fe2347

CHANGES: - Port PPX from @andreypopp/ppx_deriving_json ([melange-community/melange-json#10](melange-community/melange-json#10))

avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024

[new release] melange-json (1.2.0)

b3a7e95

CHANGES: - Port PPX from @andreypopp/ppx_deriving_json ([melange-community/melange-json#10](melange-community/melange-json#10))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port ppx from andreypopp/ppx_deriving #10

Port ppx from andreypopp/ppx_deriving #10

jchavarri commented Aug 14, 2024 •

edited

Loading

jchavarri commented Aug 15, 2024

andreypopp commented Aug 15, 2024

jchavarri commented Aug 15, 2024

jchavarri commented Aug 15, 2024

jchavarri commented Aug 15, 2024

andreypopp commented Aug 15, 2024

anmonteiro commented Aug 16, 2024

anmonteiro commented Aug 16, 2024

andreypopp commented Aug 16, 2024 •

edited

Loading

andreypopp commented Aug 16, 2024

jchavarri commented Aug 16, 2024

jchavarri commented Aug 16, 2024 •

edited

Loading

andreypopp commented Aug 16, 2024

andreypopp commented Aug 16, 2024

jchavarri commented Aug 16, 2024 •

edited

Loading

andreypopp left a comment

Port ppx from andreypopp/ppx_deriving #10

Port ppx from andreypopp/ppx_deriving #10

Conversation

jchavarri commented Aug 14, 2024 • edited Loading

Proposal

Questions

TODO

jchavarri commented Aug 15, 2024

andreypopp commented Aug 15, 2024

jchavarri commented Aug 15, 2024

jchavarri commented Aug 15, 2024

jchavarri commented Aug 15, 2024

andreypopp commented Aug 15, 2024

anmonteiro commented Aug 16, 2024

anmonteiro commented Aug 16, 2024

andreypopp commented Aug 16, 2024 • edited Loading

andreypopp commented Aug 16, 2024

jchavarri commented Aug 16, 2024

jchavarri commented Aug 16, 2024 • edited Loading

andreypopp commented Aug 16, 2024

andreypopp commented Aug 16, 2024

jchavarri commented Aug 16, 2024 • edited Loading

andreypopp left a comment

Choose a reason for hiding this comment

jchavarri commented Aug 14, 2024 •

edited

Loading

andreypopp commented Aug 16, 2024 •

edited

Loading

jchavarri commented Aug 16, 2024 •

edited

Loading

jchavarri commented Aug 16, 2024 •

edited

Loading