-
-
Notifications
You must be signed in to change notification settings - Fork 56
Deprecate deriving abstract
and add deriving dynamicKeys
#979
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
Conversation
0e64264
to
80c554d
Compare
Co-authored-by: Andrey Popp <8mayday@gmail.com>
|
||
$ dune build ./.x.objs/melange/x.cmj | ||
File "x.ml", line 3, characters 8-25: | ||
3 | let t = chartDataItemType ~height:2 ~foo:"bar" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I think it makes sense to deprecate I'm not a big fan of
Will keep thinking about some more |
ReScript called this functionality "optional fields". What about 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 Regarding casing, i chose camelcase because that's what |
I've commented on this on Discord already so just repeating here. I think 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 |
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
Your naming suggestion only accounts for the cases where |
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 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?
Then, they could be applied as a tuple |
I like this idea, here's a prototype: #987 I named them |
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 introducesdynamicKeys
.