-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix disabling utf-8 validation for nested types #148
Conversation
@@ -92,8 +104,10 @@ func (v *disableUtf8Validation) visit(n ast.Node) bool { | |||
newElts := make([]ast.Expr, len(newArr)) | |||
for i, v := range newArr { | |||
newElts[i] = &ast.BasicLit{ | |||
Kind: token.INT, | |||
Value: fmt.Sprintf("%#02x", v), | |||
// steal positions from original list to preserve newlines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read through a bunch of the formatter/printer.. it looks like adding newlines in exactly the right places (and specifically adding more than there were before) is quite a lot more work. this is a pretty easy way to just get some newlines in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but would personally like to see one test here and one in Go SDK:
- A test somewhere here but outside of the generator (i.e. in
temporalproto
package or something) that confirms, reusing one or more of our proto types, that string fields, map string keys, repeated strings, and a string in a nested message all marshal and unmarshal non-utf-8 as expected - A test in Go SDK that confirms end-to-end that you can set something like a header key and/or a client identity to a non-utf8 string and that it works all the way through server and back on history.
(but neither are blocking)
I plan to add to add tests all the way through our stack (sdk, server, etc) to make sure that everywhere we regenerate/use protos that we support this behavior |
What changed?
Fix #147 for nested types
Why?
See #147
How did you test it?
Also added unit test (just for rewrite code itself, not for effects)