-
Notifications
You must be signed in to change notification settings - Fork 884
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
Untangle ObjectValue and ObjectValueBuilder #2970
Conversation
If ObjectValue doesn't depend on ObjectValueBuilder and doesn't directly offer the ability to extract field masks, then we can use ObjectValue without pulling in SortedMap and SortedSet.
Binary Size ReportAffected SDKs
Test Logs
|
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.
LGTM, with one minor suggestion to consider.
/** | ||
* Returns a FieldMask built from all fields in an MapValue. | ||
*/ | ||
export function extractFieldMask(value: api.MapValue): FieldMask { |
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.
Consider changing the argument of extractFieldMask()
from api.MapValue
to ObjectValue
. With this change, the call sites become more readable. For example,
extractFieldMask(objValue.proto.mapValue!);
becomes
extractFieldMask(objValue);
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.
This was how the original call worked, but it used one level of indirection. The method is called recursively and only the first level uses an ObjectValue. I debated passing in just an api.Value, which would act as a sort of compromise. It however hides the fact that the method only works on a MapValue.
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.
Ahh I see. Feel free to disregard this comment then. The original way you implemented it makes sense.
If ObjectValue doesn't depend on ObjectValueBuilder and doesn't directly offer the ability to extract field masks, then we can use ObjectValue without pulling in SortedMap and SortedSet.