Fix json string encoding #122
Conversation
|
Nice one 👍 |
| assert.Equal(t, `"quotes\"nested\"in\"quotes\""`, doStrMarshal(`quotes"nested"in"quotes"`)) | ||
| assert.Equal(t, `"\u0000"`, doStrMarshal("\u0000")) | ||
| assert.Equal(t, `"\u0000"`, doStrMarshal("\u0000")) | ||
| assert.Equal(t, "\"\U000fe4ed\"", doStrMarshal("\U000fe4ed")) |
There was a problem hiding this comment.
nit/curios: why double quotes around expected val here?
There was a problem hiding this comment.
Because raw strings can't have escaped literals, and I didn't want the raw unicode fe4ed in the source document.
There was a problem hiding this comment.
I was just pointing out that you are using double quotes around expected string in the last test case vs. back ticks for all other test cases. It seems semantically it is all the same.
There was a problem hiding this comment.
graphql/string.go
Outdated
| start := 0 | ||
| io.WriteString(w, `"`) | ||
|
|
||
| for i := 0; i < len(s); i++ { |
graphql/string.go
Outdated
| @@ -6,9 +6,41 @@ import ( | |||
| "strconv" | |||
| ) | |||
|
|
|||
| const alphabet = "0123456789ABCDEF" | |||
There was a problem hiding this comment.
just a small thing but maybe call this unicodeAlphabet or something similar as I could see someone looking at the var name and wondering why it doesn't encompass the whole alphabet.
|
I can confirm that my test repo no longer panics with this fix applied and it returns the string as intended. I looked through everything else and it seems ok. I'm not really an expert on string handling though so I can't say I feel qualified to review that part. |
vektah
force-pushed
the
json-encoding-fixes
branch
from
2464432 to
f207c62
Compare
Fix json string encoding
Fixes #121
Includes a drive by fix for a panic when trying to handle encoding errors.
review appreciated @Teddy-Schmitz