Display line number on JSON errors #25038
Conversation
|
@smarterclayton @Kargakis PTAL |
k8s-github-robot
added
size/S
|
Quick unit test please |
| js := string(data) | ||
| start := strings.LastIndex(js[:syntax.Offset], "\n") + 1 | ||
| line := strings.Count(js[:start], "\n") | ||
| return fmt.Errorf("json: line %d: %s", line, syntax.Error()) |
There was a problem hiding this comment.
Can I get this error while editing a yaml file for example? If so, drop the json prefix.
There was a problem hiding this comment.
Is this decoder used by edit?
There was a problem hiding this comment.
If so, drop the json prefix.
Better yet, make it say yaml or json appropriately.
There was a problem hiding this comment.
@lavalamp @Kargakis I added 2 unit tests for broken YAML and broken JSON. The NewYAMLOrJSONDecoder seems to do good job about deciding what parser will be used and what prefix the error line will get. I think the only difference is that the YAML parser reports errors natively where the JSON not. The error above matches the YAML parser for consistency.
@Kargakis I think it is, but the error should be reporting appropriately for given format.
There was a problem hiding this comment.
Oh, I see. I missed the part where you typecast to a json decoder.
mfojtik
force-pushed
the
json-line
branch
2 times, most recently
from
266ac4c to
ce8ba42
Compare
k8s-github-robot
added
size/M
|
@smarterclayton unit test added. |
| if err == nil { | ||
| t.Fatal("expected error with json: prefix, got no error") | ||
| } | ||
| if !strings.HasPrefix(err.Error(), "yaml: line 1:") { |
There was a problem hiding this comment.
Shouldn't this error be on line 3?
There was a problem hiding this comment.
yes, it looks so, but I haven't touched the yaml parser... it seems the bug is in yaml.... I will create follow up issue?
There was a problem hiding this comment.
@lavalamp actually I think the problem was tabs on the second line, I fixed this case... however yaml still reports line '2' where it should be '3' (I guess it does not count '---' as 'line'.
There was a problem hiding this comment.
It does not
On Fri, May 6, 2016 at 7:12 AM, Michal Fojtik notifications@github.com
wrote:
In pkg/util/yaml/decoder_test.go
#25038 (comment)
:@@ -125,6 +126,40 @@ stuff: 1
}
}+func TestDecodeBrokenYAML(t *testing.T) {
- s := NewYAMLOrJSONDecoder(bytes.NewReader([]byte(`---
- stuff:
+broken-indent: 1
+---- `)), 100)
- obj := generic{}
- err := s.Decode(&obj)
- if err == nil {
t.Fatal("expected error with json: prefix, got no error")- }
- if !strings.HasPrefix(err.Error(), "yaml: line 1:") {
@lavalamp https://github.com/lavalamp actually I think the problem was
tabs on the second line, I fixed this case... however yaml still reports
line '2' where it should be '3' (I guess it does not count '---' as 'line'.—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/25038/files/ce8ba423ddf64050c4daed6548fa8794e2a4e282#r62316695
|
@smarterclayton bump |
|
cc @kubernetes/kubectl |
bgrant0607
added
area/kubectl
ok-to-merge
release-note
|
LGTM @lavalamp may want to have a look |
bgrant0607
added
the
priority/backlog
|
LGTM |
lavalamp
added
the
lgtm
|
GCE e2e build/test passed for commit 543a8b2. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit 543a8b2. |
|
Automatic merge from submit-queue |
…to-release-4.2 Bug 1840872: UPSTREAM: 88440: Report deleted pod status correctly Origin-commit: 77869edca67678ff9f6575094bae0ede1314f989
Related issue: #12231
This PR will introduce line numbers for all JSON errors in the CLI:
(this is existing error reporting for YAML)
(this is error reporting proposed in this PR for JSON)
(and this is the current reporting:)