Skip to content

Conversation

jchavarri
Copy link
Member

@jchavarri jchavarri commented Dec 11, 2023

Previously based on simplify-derive-abstract (#978).

As discussed on Discord, this PR deprecates deriving abstract because the PPX doesn't create an abstract type anymore, so the current name is misleading.

To provide an alternative, it adds deriving dynamicKeys. The name is a suggestion, if there are ideas for better naming please share.

The first commit deprecates abstract, the second introduces dynamicKeys.

Co-authored-by: Andrey Popp <8mayday@gmail.com>
Base automatically changed from simplify-derive-abstract to main December 11, 2023 11:45

$ dune build ./.x.objs/melange/x.cmj
File "x.ml", line 3, characters 8-25:
3 | let t = chartDataItemType ~height:2 ~foo:"bar"
Copy link
Member

@anmonteiro anmonteiro Dec 12, 2023

Choose a reason for hiding this comment

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

this location will be a lot better if you add a proper location on the attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure I understand your suggestion. The deriver has no access to the original ast (which is where the attribute is), it can only insert the deprecation alert on the newly created ast nodes. I believe it's the same root cause that prevented making the type abstract originally when the deriver was introduced.

More generically, it feels that implementing deriving abstract with a pure deriver was a too limiting choice for the use case.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know what I was thinking, tbh. You're right, of course.

@anmonteiro
Copy link
Member

I think it makes sense to deprecate abstract, indeed, since we don't make the type abstract anymore.

I'm not a big fan of dynamicKeys, though. Here are some other alternatives:

Will keep thinking about some more

@jchavarri
Copy link
Member Author

ReScript called this functionality "optional fields". What about optionalKeys? I think it's important to keep the naming close to the JS side (in this case "keys", because that's the part that differs from plain records). "Fields" makes me think about the record, but this deriver is not about records really.

I insist on "optional keys" or "dynamic keys" because the only real use case that remains for this deriver is the ability to create JS objects where the keys can be defined or not based on the labeled args passed to the construction function (analog to ReScript optional fields). I think making that only use case relevant through the naming will help users identify if they have to use it or not. While names like js or getters_setters don't tell anything about this use case.

Regarding casing, i chose camelcase because that's what jsConverter and newType were using. I think we should be consistent, one way or the other.

@andreypopp
Copy link
Contributor

I've commented on this on Discord already so just repeating here. I think *Keys doesn't tell what this deriver is doing.

It's generating a constructor + various setters/getters for the record, the closest thing is JaneStreet's https://github.com/janestreet/ppx_fields_conv.

I think something like @@deriving record_constructor would be more clear.

@anmonteiro
Copy link
Member

I insist on "optional keys" or "dynamic keys" because the only real use case that remains for this deriver is the ability to create JS objects where the keys can be defined or not based on the labeled args passed to the construction function (analog to ReScript optional fields).

Don't you think that the naming for this functionality should be independent of whether there are other mechanisms to achieve some overlapping use cases?

FWIW, here's what [@@deriving abstract] currently does:

  • generates a constructor for the record type it is derived from
  • generates getters and setters (if fields are mutable) for its fields
  • allows omitting entire keys from the resulting JS object if fields are marked with [@mel.optional]

Your naming suggestion only accounts for the cases where [@mel.optional] is present. IMO it fails to convey all the other generated getters / setters and constructor functions.

@jchavarri
Copy link
Member Author

The getters and setters are orthogonal to the feature we're discussing (the constructor), they were added originally because the type was abstract, but they could be implemented as a separate deriver. These getters and setters might be getting in the way throughout this conversation, so let's assume they're not part of the deriver for the rest of this comment.

The most useful functionality of the deriver (imo) is to allow producing JS objects from records that conditionally include the keys depending if the record field optional value is None or Some _. This behavior differs from the regular behavior of records in Melange, where fields with None will end up with the property in the resulting object being set, with value undefined.

This is why I think including "record" in the name is misleading, and why somehow referring to the change of behavior would help to understand what this deriver is about.

What about splitting the deriver in two?

  • deriving optional_keys (or deriving construct_optional_keys)
  • deriving getters_setters

Then, they could be applied as a tuple deriving optional_keys, getters_setters.

@anmonteiro
Copy link
Member

What about splitting the deriver in two?

  • deriving optional_keys (or deriving construct_optional_keys)
  • deriving getters_setters

Then, they could be applied as a tuple deriving optional_keys, getters_setters.

I like this idea, here's a prototype: #987

I named them make_opt_keys and getters_setters, but we should give them more descriptive names, probably including js_ somewhere (since they're really deriving JS objects).

@anmonteiro anmonteiro merged commit fb65110 into main Dec 17, 2023
@anmonteiro anmonteiro deleted the derive-dynamicKeys branch December 17, 2023 17:49
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