simon0-o commented on PR #2469: URL: https://github.com/apache/thrift/pull/2469#issuecomment-1205201801
> Feedback on the compiler generated go code: > > 1. I thought we agreed on [THRIFT-5423: Support go parameter validation in IDLÂ #2469 (comment)](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)? Thanks for your quick reply. I have fixed the runtime error that leads to travis ci fail. I have added a new exception type `VALIDATOR_CHECK_FAILED` to application exceptions, and use it as an exception when a validation rule check fails. For 2 && 3, thanks for your suggestion, now we will use `{struct name}.{field name}` in validator exception and validator will break early in `vt.in`. About 4, I will try to optimize this part, or we can do this in the later version. About 5, in my view it should be called explicitly. If it is called by `Read`/`Write`, it could cause serialization /deserialization to fail. -- 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]
