Refactor InvalidSchemaError to support multiple errors #246
Conversation
| /// | ||
| /// Technically, this is a smart pointer that (rather than managing the ownership or memory of the | ||
| /// wrapped value) encodes Ion Schema version information along with the wrapped value. |
There was a problem hiding this comment.
❗ This was an out of date comment. Versioned is no longer a "smart pointer".
| source_location: IslSourceLocation, | ||
| } | ||
| impl PartialEq for Import { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.schema_id == other.schema_id && self.type_import == other.type_import | ||
| } | ||
| } |
There was a problem hiding this comment.
❗ source_location is extra metadata, not part of the semantic meaning of an Import, so we need a custom implementation of PartialEq to exclude it.
| source_location: IslSourceLocation, | ||
| } | ||
| impl PartialEq for TypeDefinition { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.constraints == other.constraints && self.open_content == other.open_content | ||
| } |
There was a problem hiding this comment.
❗ As with Import, source_location is extra metadata in TypeDefinition, not actually part of the equality, so we need a custom implementation of PartialEq.
| source_location: IslSourceLocation, | ||
| // Not exposed in public API. Initially `None`, but changed to `Some(_)` when resolving the schemas | ||
| resolved_type_coordinates: Option<TypeCoordinates>, | ||
| } | ||
| impl PartialEq for TypeReference { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.schema_id == other.schema_id && self.type_name == other.type_name | ||
| } | ||
| } |
There was a problem hiding this comment.
❗ Again, source_location is extra metadata, not actually part of the equality, so we need a custom implementation of PartialEq. In this case, it also exposed that the resolved_type_coordinates are not part of the "equality" definition either—two type references should be equivalent if they refer to the same type, regardless of whether one or both have been resolved.
| One(InvalidSchemaDetails), | ||
| Many(BTreeSet<InvalidSchemaDetails>), | ||
| } | ||
| impl Display for InvalidSchemaErrorQuantity { |
There was a problem hiding this comment.
❗ When there are multiple errors in an InvalidSchemaError, they are displayed like this:
foo/bar.isl:24:13 > range start must not be greater than range end
foo/bar.isl:105:22 > top level type missing name
foo/baz.isl:1:1 > unknown schema version 3.14
There was a problem hiding this comment.
Love this new Display for error reporting!
|
|
||
| /// Collects multiple errors to report them together. | ||
| /// | ||
| /// ```ignore |
There was a problem hiding this comment.
❗ This is ignore because doc tests can only use things that are visible outside of the crate, and InvalidSchemaErrorCollector is only pub(crate).
There was a problem hiding this comment.
❗ When reviewing this file, you should not view changes from all commits. Exclude the first commit of the PR (the commit that moves the file from result.rs to mod/result.rs) to see how the content of the result mod has actually changed.
There was a problem hiding this comment.
❗ If you look at only the first commit of this PR, you will see that this file was moved to result/mod.rs.
| use crate::violation::Violation; | ||
| use ion_rs::IonError; | ||
| use std::convert::Infallible; | ||
| use std::fmt::{Debug, Write}; |
There was a problem hiding this comment.
I'll fix this before merging.
| use std::fmt::{Debug, Write}; | |
| use std::fmt::Debug; |
| /// A macro that checks some condition required to be valid ISL. | ||
| /// | ||
| /// If invalid, returns an InvalidSchemaErr with the given error message. | ||
| #[macro_export] |
There was a problem hiding this comment.
❗ This internal-only macro was inappropriately being exported by the crate. I've replaced the #[macro_export] with a pub(crate) use instead.
| /// ``` | ||
| #[derive(Debug, Default)] | ||
| pub(crate) struct InvalidSchemaErrorCollector { | ||
| errors: BTreeSet<InvalidSchemaDetails>, |
There was a problem hiding this comment.
(Question) Why are we using a BTreeSet here. I don't think the errors collected has to be sorted unless I am missing something here.
There was a problem hiding this comment.
There are two different parts to this question:
I don't think the errors collected has to be sorted
In order to have a better user experience, they are sorted by schema_id, row, and column. Even though reading the ISL may occur in order and errors would already be sorted, resolving the imports and type references is a separate step (and itself includes multiple steps) so errors are not sorted by default.
Why are we using a BTreeSet here
Because I didn't think it was appropriate to allocate a vec to hold a sorted copy of the error details in the Display impl. The other option is to sort them as they are aggregated, for which the BTreeSet is the natural choice. This, however, is an implementation detail that can change if necessary.
| One(InvalidSchemaDetails), | ||
| Many(BTreeSet<InvalidSchemaDetails>), | ||
| } | ||
| impl Display for InvalidSchemaErrorQuantity { |
There was a problem hiding this comment.
Love this new Display for error reporting!
Issue #, if available:
#228
Description of changes:
resultmodule to...InvalidSchemaErrorInvalidSchemaErrorIoErrorformat!when callinginvalid_schema_error.This change looks big, but a lot of the diff is swapping various
invalid_schema_errorfor the newinvalid_schema!macro.See comments in the individual files to guide your attention to the important parts. If a file does not have a "❗" comment, then the changes are just switching from
invalid_schema_error(format!(...,invalid_schema_error_raw(format!(..., etc. to the newinvalid_schema!macro (and consequentcargo fmtchanges).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.