-
Notifications
You must be signed in to change notification settings - Fork 12
Proto command messages validation #122
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
Conversation
Temporary remove a TODO comment "check the 1st field in commands" because it is not required to have an entity ID as the first field in commands if the command is not for an entity.
Move validation tests commands.proto
Move command validation method to the separate class.
Current coverage is
|
| string msg = 2; | ||
| } | ||
|
|
||
| // TODO:2016-03-14:alexander.litus: is this option really needed? Why not use min/max instead? |
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.
Let's address this and other TODOs.
| message ConstraintViolation { | ||
| // An interpolated error message for this constraint violation. | ||
| // An error message for this constraint violation | ||
| // (or a template which contains `%s` format specifiers). |
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.
If it contains format placeholders, then it's not a message, it's message_format, or, better, format.
|
@alexander-yevsyukov, PTAL. It is remained to implement the Gradle plugin for generating entities.props. And the build will pass then. |
Conflicts: client/src/test/java/org/spine3/base/CommandsShould.java
|
@alexander-yevsyukov, PTAL. I've requested globally unique field numbers for our custom options, it is remained only to add them. |
|
LGTM |
No description provided.