ppx: add [@drop_default] for record fields

[andreypopp]

For now it only works for record fields annotated with [@option], by dropping the field from JSON repr when the record value is None.

What's missing is to also support [@drop_default] for fields annotated by [@default X] but we need to decide how to check for equality between the default value and the field value.

One nice idea I had is to generate code like this:

type t = { a : int [@default 0] [@drop_default] }
...
let bnds =
  match [%equal int] a 0 with
  | true -> bnds
  | false -> ("a", a)::bnds
in
...

but this means this ppx will depend on another ppx which provides [%equal t] deriver but sadly ppx_compare doesn't work with melange now.

[anmonteiro]

What's missing is to also support [@drop_default] for fields annotated by [@default X] but we need to decide how to check for equality between the default value and the field value.

I honestly don't think this is necessary at all, or at least I can't see a use case. Do you have an example where you'd want to use [@drop_default] but not an option type?

[andreypopp]

I honestly don't think this is necessary at all, or at least I can't see a use case. Do you have an example where you'd want to use [@drop_default] but not an option type?

If your field type is not _ option, e.g. : int [@default 0], : _ list [@default []] and so on. I think it's useful and atd and ppx_conv_yojson provide similar mechanisms.

But I agree that dropping the None is probably the most often used, hence I suggest to add this feature only for [@option] and then later see if we want to extend it further.

[anmonteiro]

Suggested change

	let k = ld.pld_name in
	let k = Option.value ~default:k (ld_attr_json_key ld) in
	let k =
	let k = ld.pld_name in
	Option.value ~default:k (ld_attr_json_key ld) in

[anmonteiro]

I kinda like this pattern, could also use it for v, but this is optional if you don't like the extra nesting

[jchavarri]

Could you add a changelog entry please?

I think I'd design the API differently to avoid corner cases and make it less verbose. We could do [@option drop_default] now, and in the future potentially [@default 0, drop].

[jchavarri]

What happens now if we use drop_default in a non option field? Also, would it be worth to test this case? To have it documented "internally".

[andreypopp]

added a test case

[jchavarri]

Is it option (line 254) or json.option?

[andreypopp]

It’s both, in ppxlib you register attributes fully qualified but they can be used both ways.

[andreypopp]

Could you add a changelog entry please?

done

I think I'd design the API differently to avoid corner cases and make it less verbose. We could do [@option drop_default] now, and in the future potentially [@default 0, drop].

[@default 0, drop] is problematic as 0, drop is a tuple expression and can be a default value

what can be done is to make dropping the defaults the default behaviours, and instead add [@keep_default] to make it emit JSON with default values

this is probably ok for [@option] but for [@default E] when we add it, it means we'll need to require equality function defined for type of E

[jchavarri]

Thanks!