Add show @printer support for polymorphic variants
#286
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gasche
approved these changes
Jun 1, 2024
There was a problem hiding this comment.
Thanks! This is a nice thing to have, and the implementation in this PR is slightly better than #267. Let's not make it wait as long...
Could you maybe include a CHANGELOG entry to credit both you and @ghuysmans, so that we don't forget to do so later?
| @@ -23,6 +23,7 @@ let attr_printer context = Attribute.declare "deriving.show.printer" context | |||
| Ast_pattern.(single_expr_payload __) (fun e -> e) | |||
| let ct_attr_printer = attr_printer Attribute.Context.core_type | |||
| let constr_attr_printer = attr_printer Attribute.Context.constructor_declaration | |||
| let rtag_attr_printer = attr_printer Attribute.Context.rtag | |||
There was a problem hiding this comment.
I was lost for a while as to what rtag means. The answer is that this obscure name comes from the parsetree, and it means "a constructor declaration in a polymorphic variant type" (r means row and tag is supposed to suggest constructor, I guess). The other PR #267 uses attr_printer field.prf_attributes directly, and I suppose that usig the ppxlib mechanism as in here is better.
sim642
added a commit
to sim642/ppx_deriving
that referenced
this pull request
Jun 2, 2024
gasche
reviewed
Jun 2, 2024
sim642
force-pushed
the
show-poly-variant-printer
branch
from
34a5bff
to
fc7e50d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This missing support is listed as one of the motivations for ppx_deriving_variant_string:
I quickly implemented this and only then discovered that #267 already exists.
The difference is that this is for the migrated ppxlib attributes and does use
wrap_printer.