Validation restructure #163
Conversation
There was a problem hiding this comment.
Looks pretty fancy to me!
I think some cleanup is in order, but with the breadth of this PR, probably better to do it separately.
Also, I think you should change your functions to take an ErrorReporter, and report directly into it, instead of keeping their own errors, and returning them. It leads to unnecessary copying, and the new function in the error reporter.
| @@ -52,6 +52,16 @@ impl ErrorReporter { | |||
| self.report(message.into(), location, ErrorLevel::Warning); | |||
| } | |||
|
|
|||
| pub fn report_errors(&mut self, errors: &[Error]) { | |||
There was a problem hiding this comment.
This feels a little weird to me, making clones of every error, then adding the clones to the array.
I understand why you had to though, since report takes the pieces, and not the errors themselves.
Hopefully Joe's changes to error reporting make this function unnecessary, but if not, we should probably change report to just take an error, and change this to take an owned Vec<Error> instead.
There was a problem hiding this comment.
Will be addressing this in a follow-up PR where I cleanup some of the names too!
| fn visit_operation_start(&mut self, operation_def: &Operation) { | ||
| self.validate_stream_member(operation_def.parameters()); | ||
| self.validate_stream_member(operation_def.return_members()); | ||
| fn visit_parameter(&mut self, parameter: &Parameter) { |
There was a problem hiding this comment.
The visitor visits parameters and return_members differently.
This only visits parameters at the moment, so probably want an identical visit_return_member function too.
There's a chance that this distinction isn't useful anymore, so we can maybe change this behavior. But for now, your validator isn't validating any return members.
There was a problem hiding this comment.
Will make an issue!
| /// Validate that the enumerators are within the bounds of the specified underlying type. | ||
| fn backing_type_bounds(enum_def: &Enum) -> ValidationResult { | ||
| let mut errors = vec![]; | ||
| if enum_def.supported_encodings().supports(&Encoding::Slice1) { |
There was a problem hiding this comment.
This is unrelated to the PR, but this check is incorrect. An enum isn't limited to supporting 1 encoding, or the other. It can support both.
encoding = 1;
enum MyEnum { Foo }
This enum is supportable by both Slice1 and Slice2, but we're only validating the Slice1 side.
There was a problem hiding this comment.
Looks good. Just need to fix @InsertCreativityHere's comments.
| Validate::Attributable(validate_compress_attribute), | ||
| Validate::Operation(validate_format_attribute), | ||
| Validate::Parameter(validate_deprecated_parameters), | ||
| Validate::Members(validate_deprecated_data_members), |
There was a problem hiding this comment.
For a followup PR, I think names should be more descriptive. Eg.
Validate::Members(can_not_be_marked_as_deprecated),
No description provided.