ppx: derive signatures #32
Conversation
| @@ -11,11 +11,16 @@ class virtual deriving : object | |||
| loc:location -> path:label -> core_type -> expression | |||
| (** a deriver can be applied to as type expression as extension node. *) | |||
|
|
|||
| method virtual generator : | |||
| method virtual str_type_decl : | |||
| ctxt:Expansion_context.Deriver.t -> | |||
| rec_flag * type_declaration list -> | |||
| structure | |||
| (** or it can be attached to a type declaration. *) | |||
There was a problem hiding this comment.
Off-topic, but I don't understand this comment.
| | Ptyp_var txt -> { txt; loc } | ||
| | _ -> | ||
| Location.raise_errorf ~loc | ||
| "type variable is not a variable" |
There was a problem hiding this comment.
Maybe something like:
| "type variable is not a variable" | |
| "unsupported type description: only type variables such as `'a` are supported" |
(feel free to ignore, i saw this error msg was used already)
| let name = | ||
| match t.ptyp_desc with | ||
| | Ptyp_var txt -> { txt; loc } | ||
| | _ -> |
There was a problem hiding this comment.
Why do derive_of_type_declaration and gen_type_ascription support Ptyp_any but not here?
Edit: oh, because we need a name to keep drilling I guess 🤔
|
@andreypopp is this good to merge? none of my comments is blocking. |
|
Ah, we're missing a changelog entry I guess. |
|
Actually I think this doesn’t work properly in case you are extending a polymorphic variant decoder. This is because it generates an extra function with _poly suffix which is not being exposed in signatures. |
| @@ -732,52 +586,6 @@ module Conv = struct | |||
| :: next) | |||
| in | |||
| pexp_match ~loc x cases | |||
|
|
|||
| method! derive_of_type_declaration td = | |||
There was a problem hiding this comment.
what exactly are we deleting? aren't we reducing the scope of what's supported?
There was a problem hiding this comment.
i thought we would be losing the polyvar composition, but from what i could check (i tested the test in the comment below), we don't, the test keeps passing fine.
There was a problem hiding this comment.
I’m going to work a bit more on the last commit but the gist is — instead of generating a separate function for polyvariants which decodes into None on unmatched tag, we extend error type to have a case for unmatched tag. This is why we removed code here.
src/Json_decode.mli
Outdated
| @@ -13,7 +13,11 @@ third-party libraries. | |||
| type 'a decoder = Js.Json.t -> 'a | |||
| (** The type of a decoder combinator *) | |||
|
|
|||
| exception DecodeError of string | |||
| type error = Json_error of string | Unpexpected_variant of string | |||
There was a problem hiding this comment.
Doesn't seem that the second branch of the variant is used, but:
| type error = Json_error of string | Unpexpected_variant of string | |
| type error = Json_error of string | Unexpected_variant of string |
There was a problem hiding this comment.
The second case is generated by ppx
|
I think this can be merged once we figure out the issue with |
It's not longer generic, hardcodes JSON specifics but that's ok.
|
ftr |
|
yeah, that's a breaking change... |
No description provided.