-
Notifications
You must be signed in to change notification settings - Fork 132
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
Improve Leaf Record Errors #360
base: development
Are you sure you want to change the base?
Conversation
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 like the idea, but I think if we want to capture the unknown / unsupported type records, then we should be consistent with this and use it everywhere. E.g., SerializedArrayTypeRecord could benefit from the same type of construction.
However, simply replacing code like this with a call to the proposed GetLeafRecord
method loses contextual information if an error occurs. E.g., there is a difference between a message such as SerializedArrayTypeRecord contains an incorect leaf type at index 01234.
and Array type 5678 contains an invalid element type index 1234.
. The second one is more specific, since it also indicates which property failed to be parsed, rather than simply stating that some index within the leaf was invalid. Furthermore, the first does not include the type index of the faulty leaf itself either.
Can we somehow rephrase / refactor this such that we do not lose this contextual information?
How do you want to handle the |
@ds5678 The array type record was just an example of how a leaf could reference multiple type indices, and thus it would be unclear in the error message of the proposed change which type index was invalid. Some simple idea would be to add a string parameter to the |
No description provided.