-
Notifications
You must be signed in to change notification settings - Fork 7
Return all validation errors #20
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
base: main
Are you sure you want to change the base?
Conversation
I don't think that needs to be dynamic.
I used prost-validate/prost-validate-derive-core/src/derive.rs Lines 106 to 121 in 4ad7978
|
b23307d to
faa2bf1
Compare
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.
Thank you very much 😀
Maybe we need to add the method in the example and update the README accordingly.
It also needs to be implemented in the prost-reflect-validatecrate.
prost-validate/src/lib.rs
Outdated
| pub fn validate_all(&self) -> Vec<Error> { | ||
| Validator::validate_all(self.0) | ||
| } |
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 think it would be better to return a Result instead of just a vector of errors. Using Result clearly indicates whether validation succeeded or failed and allows users to handle failures idiomatically with the ? operator. Also, returning an error type that implements From<Error> for tonic::Status makes integration with tonic seamless.
4e7ee17 to
7e6307a
Compare
Signed-off-by: Pierre Fenoll <[email protected]>
Closes #6
WIP:
Errorstype that is a vector of Error as the encapsulation withprost_validate::Errorrarely makes sense.Any idea on what
cargoinvocation I can pull to show the final generated code? Looking inside./targetisn't helping. Thanks!