fishy commented on PR #2469: URL: https://github.com/apache/thrift/pull/2469#issuecomment-1204551295
Feedback on the compiler generated go code: 1. I thought we agreed on https://github.com/apache/thrift/pull/2469#discussion_r858950006 to use `TApplicationException` for validation errors, but the example is still using `errors.New` 2. In error messages the `p.` prefix doesn't really bring any extra value, it would be more useful to use the type name instead of `p` (example: "BasicTest.Byte0 not valid, ...") 3. (minor) For `vt.in` check you can break the for loop early after first find 4. (minor) For `vt.pattern` check `regexp.MatchString` calls would be a performance nightmare because it needs to compile the pattern on every check. It's probably OK for the first version but we should consider declare global variables to compile the patterns defined in thrift files. 5. Is `IsValid` called by `Read`/`Write` or does it need to be called explicitly (e.g. manual check only)? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
