feat: add JSON functions that work with dynamic values#5259
Conversation
wolffcm
requested review from
lwandzura and
nathanielc
and removed request for
a team
wolffcm
changed the title
nathanielc
left a comment
nathanielc
left a comment
There was a problem hiding this comment.
LGTM, with two small fixes
sanderson
left a comment
sanderson
left a comment
There was a problem hiding this comment.
Some minor doc suggestions.
wolffcm
force-pushed
the
feat/dynamic-json-parse
branch
from
a00c57b to
2948d80
Compare
onelson
left a comment
onelson
left a comment
There was a problem hiding this comment.
Looks great. Love to see these good tests!
| arr := v.Array() | ||
| a := make([]interface{}, arr.Len()) | ||
| var rangeErr error | ||
| arr.Range(func(i int, v values.Value) { |
There was a problem hiding this comment.
I wondered whether I should use Array.Range or just use a plain loop while I was looking at asArray. I opted for "plain loop" just because it seemed more minimal. What made you prefer it?
My guess: in this function we're iterating over different collection types (array, dict, object) and the Range() method might make this work more uniform.
There was a problem hiding this comment.
This code I mostly copied from the non-dynamic implementation of encode. I only had to add the case to semantic.Dynamic above. So it was like this already. Looking at the alternative, I can't say that I have a strong preference.
I considered just changing the regular version of json.encode to be able to handle dynamic, since it's an additive change that shouldn't break with existing behavior, but decided it would be more prudent to copy it.
4 participants
This PR adds two new functions to the
experimental/dynamicstdlib package:jsonParsewhich produces dynamic values.jsonEncodewhich encodes JSON bytes into dynamic values.I had to cherry-pick a commit from #5239 to fix type inference to allow calling functions that accept
dynamicarguments. I also found an issue wheremapdoes not deal withdynamictypes, but this was an easy one-line fix.Fixes #5141.
Checklist
Dear Author 👋, the following checks should be completed (or explicitly dismissed) before merging.
experimental/docs/Spec.mdhas been updated (N/A)Dear Reviewer(s) 👋, you are responsible (among others) for ensuring the completeness and quality of the above before approval.