-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
std.json: Adds jsonParse to override parsing behavior #8791
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
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.
implementing this was always planned, but its tricky because you need to handle the token lookahead. Trying to do that in a way that isn't a foot-gun for people implementing the trait was too difficult IMO.
I was planning on an eventual overhaul of the token parser, so I had delayed implementing this feature (as it would break everyone's traits).
@@ -2648,17 +2673,9 @@ pub fn stringify( | |||
} | |||
}, | |||
.Enum => { | |||
if (comptime std.meta.trait.hasFn("jsonStringify")(T)) { |
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.
only enum/union/struct can have user traits; it makes more sense to keep them here?
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 is exactly why I found it reasonable to move it outside: it stops this check from getting repeated in the code without altering behavior. Since I applied the same reasoning to jsonParse
and jsonStringify
is effectively its pair I thought it was a good idea to have the same logic applied to it.
@@ -1708,6 +1708,9 @@ fn parseInternal(comptime T: type, token: Token, tokens: *TokenStream, options: | |||
} | |||
|
|||
pub fn parse(comptime T: type, tokens: *TokenStream, options: ParseOptions) !T { | |||
if (comptime std.meta.trait.hasFn("jsonParse")(T)) { |
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 will only work at the top level: the internal recursion uses parseInternal
.
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.
Whoops, for all examples I tried I somehow never had one where the root did not have jsonParse
implemented. What's stopping us from simply keeping a boolean on TokenStream
to know if we need the lookahead or not?
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 have investigated, and the token parser indeed badly needs an overhaul. I couldn't find a simple enough way to get this properly done without a refactor, but I can't do that right now, so for the time being I will close this PR.
std.json
currently has the ability to override serialization behavior by implementingjsonStringify
. This is pretty useless, as the other way is impossible. As such I addedjsonParse
to override deserialization.I also unified all
jsonStringify
s into a single one at the beginning of thestringify
function.