Escape quotation marks in text encoding #152
Conversation
The switch statement [here](https://github.com/capnproto/go-capnproto2/blob/503c1bb7bbe95734920325b8999cc89b8b79fcfa/internal/strquote/strquote.go#L29-L34) is attempting to escape quotation marks, but since `needsEscape` didn't return `true` for quotation marks, they never made it into the switch statement, and therefore weren't getting escaped.
|
Looks like the Bazel build is failing but the non-Bazel build passed, although that happened on the most recent commit to master too so I don't think it's caused by this change. I'm not familiar with Bazel so I don't think I should be mucking about with the configuration, but based on these threads it sounds like it's an issue with go 1.13 and updating the versions of rules_go and gazelle might fix it. |
There was a problem hiding this comment.
Changes are simple, so I'm okay with merging this even though I don't understand the details. Could you just fill me in a little bit?
P.S.: sorry this took so long to review. zenhack, taylorjdawson and myself are actively developing this project now, so please feel free to tag one of us in any future PR.
| @@ -59,8 +59,8 @@ func TestEncode(t *testing.T) { | |||
| {0x81fdbfdc91779421, `(map = [])`, `( | |||
| map = [] | |||
| )`}, | |||
| {0x8e85252144f61858, `(data = "Hi\xde\xad\xbe\xef\xca\xfe")`, `( | |||
| data = "Hi\xde\xad\xbe\xef\xca\xfe" | |||
| {0x8e85252144f61858, `(data = "Hi\xde\xad\xbe\xef\xca\xfe\"\'\\")`, `( | |||
There was a problem hiding this comment.
I'm okay with merging this optimistically, but could you just give me a bit of context about what's going on here? The change seems very simple, but I'm not familiar with this part of the code base, so I just want to do my due diligence. 🙂
There was a problem hiding this comment.
I assume this is just expanding the test, to cover the characters relevant to this patch.
| @@ -55,7 +55,7 @@ const mapVal @0xb167974479102805 :Value = (map = [ | |||
| (key = "foo", value = (void = void)), | |||
| (key = "bar", value = (void = void)), | |||
| ]); | |||
| const data @0x8e85252144f61858 :Value = (data = 0x"4869 dead beef cafe"); | |||
| const data @0x8e85252144f61858 :Value = (data = 0x"4869 dead beef cafe 22275c"); | |||
There was a problem hiding this comment.
Same here. Can you just explain like I'm 5 what the 22275c corresponds to and why it's needed?
There was a problem hiding this comment.
This is just the hex encoding of the characters added above.
|
@zenhack Does this seem OK to merge? |
| @@ -59,8 +59,8 @@ func TestEncode(t *testing.T) { | |||
| {0x81fdbfdc91779421, `(map = [])`, `( | |||
| map = [] | |||
| )`}, | |||
| {0x8e85252144f61858, `(data = "Hi\xde\xad\xbe\xef\xca\xfe")`, `( | |||
| data = "Hi\xde\xad\xbe\xef\xca\xfe" | |||
| {0x8e85252144f61858, `(data = "Hi\xde\xad\xbe\xef\xca\xfe\"\'\\")`, `( | |||
There was a problem hiding this comment.
I assume this is just expanding the test, to cover the characters relevant to this patch.
| @@ -55,7 +55,7 @@ const mapVal @0xb167974479102805 :Value = (map = [ | |||
| (key = "foo", value = (void = void)), | |||
| (key = "bar", value = (void = void)), | |||
| ]); | |||
| const data @0x8e85252144f61858 :Value = (data = 0x"4869 dead beef cafe"); | |||
| const data @0x8e85252144f61858 :Value = (data = 0x"4869 dead beef cafe 22275c"); | |||
There was a problem hiding this comment.
This is just the hex encoding of the characters added above.
The switch statement here is attempting to escape quotation marks, but since
needsEscapedidn't returntruefor quotation marks, they never made it into the switch statement, and therefore weren't getting escaped.