feat: Implement nested JSON in api subcommand - #389
Conversation
2ca9dc4 to
291c1ca
Compare
jedevc
left a comment
jedevc
left a comment
There was a problem hiding this comment.
Sorry for my slowness! I finally got round to it (chaos week sorry) - just some early feedback, I saw this is still in draft. But it's so neat!
Much love for this new Jason type 馃帀
| var bodyData jason.Jason[map[string]any] | ||
| data := []byte(strings.Join(items, "\n")) | ||
| if err := jason.Unmarshal(data, &bodyData); err != nil { | ||
| return nil, err | ||
| } | ||
| return jason.Marshal(bodyData) |
There was a problem hiding this comment.
Instead of this, you could just loop over all items, and unmarshal them one-by-one.
So essentially:
tags = []Unmarshal("tags[]=production")tags = [production]Unmarshal("tags[]=eu-west")tags = [production, eu-west]
There was a problem hiding this comment.
the way I am thinking of it, is that, a valid JSON is [....] or {} with proper syntax that have proper key value.
On other hand a valid nested-JSON is list of key=value pairs or valid JSON
so jason.Unmarshal somehow should take all of these and parse them as a one unit, that was my vision behind that Unmarshaling.
What we can do is keep parser to expect a \n to seperate them with clear comment about expected input, but in api.go we pass items one by one instead of joining them which also results in a valid end result.
Another thing we can do is make the parser bit smarter about spaces, but that ofc will come with complexity cost depends if spaces are allowed or not. For example tags [] = production is this valid nested-JSON or not, based on that the parsing becomes complex.
5f593fa to
8e84b1f
Compare
0100d2f to
6634124
Compare
|
@jedevc the PR is out of draft and ready for review, I modified the parser to apply what I mentioned here #389 (comment) |
|
Have a copilot review 馃帀 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new internal/jason package to support HTTPie-style nested JSON (plus JSON literals) and updates the unikraft api command to accept request bodies via positional arguments (with richer help/examples) instead of -d/--data.
Changes:
- Introduces a nested-item parser that builds JSON from
key=value,key:=raw, bracket paths, and root JSON literals. - Updates
unikraft apito parse/merge positional body arguments (and deprecate-d/--data) into a JSON request body. - Expands CLI help text and examples; adds unit tests + fuzz tests for the nested JSON parser.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/jason/parse.go | Implements parsing + tree building for nested-item input and root JSON literal merging. |
| internal/jason/jason.go | Adds Jason[T] wrapper with Unmarshal fallback from JSON to nested-item syntax. |
| internal/jason/debug.go | Adds visualization/debug helpers for parsed structures. |
| internal/jason/jason_test.go | Adds comprehensive parser behavior tests (arrays, nesting, escaping, merges). |
| internal/jason/fuzz_test.go | Adds fuzzing for parseItem and buildNestedJSON. |
| internal/cmd/api.go | Deprecates -d, adds positional body args, resolves body via internal/jason. |
| cmd/unikraft/testdata/TestHelp/api | Updates golden help output to document new request body syntax and examples. |
dc0956b to
7eeccc3
Compare
Signed-off-by: ayn <ayn.khatib@gmail.com>
7eeccc3 to
fcd5226
Compare
|
Can you close the comments above that have been resolved? I'm starting to do a pass now also |
craciunoiuc
left a comment
craciunoiuc
left a comment
There was a problem hiding this comment.
Hey, left some comments
Mostly related to code comments and tests.
One more note on that: Can you add 1-2-3 more e2e tests that use this new formatting, I think our unikraft api tests are a bit simple
| // | ||
| // I first named it nested, but it's not only nested, it's nested-JSON + JSON together. | ||
| // Couldn't find a better name, jason was closest to my head because of a meme stuck in my head from youtube I guess. Prob Primeagen can't remember. | ||
| // | ||
| // why name it JASON? https://github.com/unikraft-cloud/cli/pull/389#discussion_r3524002021 |
There was a problem hiding this comment.
Remove this part 馃槶 (though I like it)
There was a problem hiding this comment.
BUT, justin asked for it.
Someone will walk out disappointed.
There was a problem hiding this comment.
It is war then
I'm also fine with calling the package justin (Just ur Usual String-Typed INput)
| package jason | ||
|
|
There was a problem hiding this comment.
Considering how much parsing Jason has, I would suggest running the robot in planning mode to expand the unit tests for it a bit. It's really easy to lose corner cases for this
There was a problem hiding this comment.
Ham, we are at Friday, maybe I can use Ultracode so it sucks what is left from weekly bandwith
There was a problem hiding this comment.
yup exactly 馃構
Though ultracode is a bit weird, after setting a prompt I always see it after set on XHigh? Not sure what is going on
So maybe try Max instead? no idea
Only request: create as a separate commit so they don't get mixed up with the things we already reviewed
There was a problem hiding this comment.
Okay, so mr robot did stuff and pushed them, mainly a bug in code pushed today.
Signed-off-by: ayn <ayn.khatib@gmail.com>
Signed-off-by: ayn <ayn.khatib@gmail.com>
craciunoiuc
left a comment
craciunoiuc
left a comment
There was a problem hiding this comment.
I actually don't have anything else against Jason
I'll let @jedevc approve
Fixes #369.
Closes #369.
Implement nested JSON in api subcommand