[WIP] Added JSON input validation for CLI commands #1771
Conversation
| summary += fmt.Sprintf("- %s\n", diag.Summary) | ||
| } | ||
| return fmt.Errorf("json input error:\n%v", summary) | ||
| } |
There was a problem hiding this comment.
Is there significance to normalizing before/after the ToTyped call?
The conversion logic now lives in ToTyped.
There was a problem hiding this comment.
@pietern ToTyped seems not to return diagnostics for, for example, unknown fields while normalise do, hence the reason for extra call
| Line: 1, | ||
| Column: 1, | ||
| } | ||
| return newLoader().load(&root, loc) |
There was a problem hiding this comment.
What would it take to have real location information here?
There was a problem hiding this comment.
@pietern this is what I am working on still, so TBD :)
| @@ -33,7 +35,27 @@ func (j *JsonFlag) Unmarshal(v any) error { | |||
| if j.raw == nil { | |||
| return nil | |||
| } | |||
| return json.Unmarshal(j.raw, v) | |||
|
|
|||
| dv, err := jsonloader.LoadJSON(j.raw) | |||
There was a problem hiding this comment.
Could we do the same for the YAML flag?
I believe that's the only reason we still depend on github.com/ghodss/yaml. This package uses the json tags in the types as opposed to yaml tags, which the upstream YAML package uses. Since we use the json tags as well in the dyn package it should be possible to replace it.
Not blocking for this PR, of course.
Changes
Added JSON input validation for CLI commands
Relies on using
dynlibrary.Fixes #1769 #1764
Tests
Added unit tests