Type Yojson.Safe.t with equal and show/pp

[Leonidas-from-XIV]

In recent years the OCaml community has converged on some conventions:

• The main type in a module is t
• There is a non-polymorphic comparison function
• There is a way to print the data type (to a lesser degree adopted but often still useful)

Yojson is odd in these regards as the type is called json (it always trips me up and I have to look it up what it is named), there is no compare function (so I have to write my own) and the pretty printing functions have non-standard names and signatures so I can't just use show and pp as ppx_deriving_show helpfully generates.

Therefore I would suggest:

• Change the type to be Yojson.Safe.t while keeping a type alias json to preserve compatibility
• Add an equal : 't -> 't -> bool function in the Safe module
• Add show/pp functions in the Safe module

I would volunteer of sending a PR, I just want to avoid spending the time implementing to have the suggestions ultimately rejected.

[hcarty]

t as an alias of json seems like a very good idea.

pp could be an alias for Yojson.Safe.pretty_print

equal is useful but a little tougher. Should it be order-independent for the pairs in `Assoc pairs?

[Leonidas-from-XIV]

I don't think it should be order-dependent, since these represent JS(ON) objects and these are not ordered as far as I am aware.

[NathanReb]

This would be a welcome change indeed!

If we are to expose an equal function I think we should also probably go for a compare to spare one's need to define it if they want to apply the Map.Make functor to Yojson's modules for instance.

I'm not sure pp should be an alias for the current pretty_print as it is far from what you would get if you derived it on your own with:

type t = [%import: Yojson.Safe.json]
[@@deriving show]

I personally use pp and show mostly for testing and debugging and I like that it is shows the "rough" ocaml representation. I'd be curious to know what the users would expect from those but I don't think it would hurt to keep them separate from serialization functions.

Anyway, if you want to have a go at it, you've got my blessing!

[Leonidas-from-XIV]

@NathanReb Glad to hear. This is also how I use pp so I am in favour of doing it that way.

I am less sure about compare because the semantics of compare are not very clear to me. Is `String greater or smaller than `Assoc etc.? I assume compare would yield 0 on differently ordered `Assoc but that that might be strange if you try to look up something in a Map. Or maybe it might be exactly what one would want.

Another question is whether this should be implemented manually or whether pulling in ppx_deriving as dependency is okay, since except for the slightly custom equal logic it is basically boilerplate.

[NathanReb]

I definitely think we should use ppx_deriving for that. Maintaining this kind of functions by hand is a liability. Even eq and compare could be derived internally and we would expose functions that sorts the objects before calling them.

I think compare should behave consistently with equal meaning in this case comparing objects with differently ordered fields should indeed return 0. We could leave the rest to ppx_deriving as it does a pretty good job with that.
Anyway we can start with equal and show/pp and think about compare later!

[mjambon]

@NathanReb A word of caution - Think about the following costs, in the scenario where you use ppx_deriving and in the scenario where you don't:

• Implementing compare.
• Documenting the order implemented by compare.
• Ensuring that yojson still compiles and works identically with all supported versions of ocaml.
• Reimplementing compare when ppx_deriving becomes obsolete.

Implementing compare by hand correctly doesn't seem like a daunting task to me compared to the other points.

[NathanReb]

I agree deriving compare and equal isn't necessarily the best of ideas since we want them to have a custom behavior. There's no easy way to do it strictly with ppx_deriving since the [@equal fun ...] doesn't work with variants as far as I can tell. I was mostly suggesting we use it for pp and show and eventually for the other two if that was convenient enough.

I don't think documenting and ensuring that it works are any tougher with ppx_deriving. Its behavior is well documented. Also I think that might be a good time to add unit tests and thoroughly test those functions. The CI would then ensure we don't break the initial behaviour.

I tend to treat lines of code as a liability and am not a huge fan of re-implementing by hand what's already been done perfectly well elsewhere if I can.

My main concern about using ppx_deriving for compare and equal here is that the order will then depend on the order of the variants in the type declaration if I'm not mistaking (although that could be different for polymorphic variants). I think as long as the tests pick it up it's an ok limitation.

Of course if you are strongly against the use of ppx_deriving I won't go against your will.

[mjambon]

This is really your call. I still have a vivid memory of rewriting a bunch of stuff due to the undocumented "new camlp4" that came around about 10 years ago, and finally reimplementing the json tools with no camlp4 at all to insure against such future problems. So I'm erring on the conservative side.

…

On Fri, Dec 21, 2018, 03:13 Nathan Rebours ***@***.*** wrote: I agree deriving compare and equal isn't necessarily the best of ideas since we want them to have a custom behavior. There's no easy way to do it strictly with ppx_deriving since the ***@***.*** fun ...] doesn't work with variants as far as I can tell. I was mostly suggesting we use it for pp and show and eventually for the other two if that was convenient enough. I don't think documenting and ensuring that it works are any tougher with ppx_deriving. Its behavior is well documented. Also I think that might be a good time to add unit tests and thoroughly test those functions. The CI would then ensure we don't break the initial behaviour. I tend to treat lines of code as a liability and am not a huge fan of re-implementing by hand what's already been done perfectly well elsewhere if I can. My main concern about using ppx_deriving for compare and equal here is that the order will then depend on the order of the variants in the type declaration if I'm not mistaking (although that could be different for polymorphic variants). I think as long as the tests pick it up it's an ok limitation. Of course if you are strongly against the use of ppx_deriving I won't go against your will. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#71 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAU84R_pjzPrNHoVX4E2YIhl6SU0EYT1ks5u7MJmgaJpZM4Za5Ii> .