Support class type declarations in derivers #538
Conversation
patricoferris
requested review from
ceastlund,
NathanReb,
panglesd and
pitag-ha
as code owners
There was a problem hiding this comment.
This looks good to me! I can't think of a good reason why not to add this.
I'm curious as to why you've added the Class_decl context and handled it everywhere without exposing it. Is this an omission or is there a particular reason why you did this that I missed?
I think only allowing it for class types is fair given that's what we do for modules as well, although I could see how it would be useful to allow deriving values from class/module declarations but I think it's a topic for another time.
| @@ -105,10 +105,12 @@ with type deriver := t | |||
|
|
|||
| val add : | |||
| ?str_type_decl:(structure, rec_flag * type_declaration list) Generator.t -> | |||
| ?str_class_type_decl:(structure, class_type_declaration list) Generator.t -> | |||
There was a problem hiding this comment.
This change could potentially break some rev deps, although I deem it quite unlikely.
I think it's fair to treat this as non breaking from our perspective given how the add and add_alias function are used in practice but still wanted for us to keep this in mind before we release this!
There was a problem hiding this comment.
Agreed.
As a litmus test, I tested this change against 50 revdeps without any failures due to this. There were failures because of flakey tests and no solves.
|
This needs a changelog entry! |
patricoferris
force-pushed
the
deriving-classes
branch
from
2ad34aa
to
64da528
Compare
Signed-off-by: Patrick Ferris <[email protected]>
patricoferris
force-pushed
the
deriving-classes
branch
from
64da528
to
f90ac1a
Compare
This PR adds support for class type declarations, which I mistook #509 for. All the same, seems like a good feature to add given ppxlib didn't support it.
It reuses some code from type declarations as they are pretty similar but there is Recursive flag for class types (are the just always recurive?). This bit could perhaps use some more work.