-
Notifications
You must be signed in to change notification settings - Fork 940
Provide document path context for input validation errors. #3189
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
Binary Size ReportAffected SDKs
Test Logs |
@@ -107,11 +107,12 @@ export class ArrayUnionFieldValueImpl extends SerializableFieldValue { | |||
|
|||
_toFieldTransform(context: ParseContext): FieldTransform { | |||
// Although array transforms are used with writes, the actual elements | |||
// being uniomed or removed are not considered writes since they cannot | |||
// being unioned or removed are not considered writes since they cannot | |||
// contain any FieldValue sentinels, etc. | |||
const parseContext = new ParseContext( |
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.
It might be time to add a helper method to ParseContext (or a tree-shakeable function in this file) that simplifies the creation of an argument parser. If arrayElement
remains configurable, this code would replace line 112-122, 145-155 and 173-181.
Optional.
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 pulled out createSentinelChildContext
. LMK if this is what you had in mind.
description += ' (found'; | ||
|
||
if (hasPath) { | ||
description += ` in field ${path!.toString()}`; |
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.
Nit: ${path!.toString()}
is the same as ${path}
(here and below).
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.
Fixed.
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.
Looks great. Thanks for the good explanation in the comment!
Potentially addresses #3169.