Rename proto_values->values, create values.test.ts #2750
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
schmidt-sebastian
force-pushed
the
mrschmidt/valuesrename
branch
from
1d020eb to
5a90ea2
Compare
schmidt-sebastian
requested review from
wilhuff and
ashwinraghav
and removed request for
ashwinraghav
schmidt-sebastian
force-pushed
the
mrschmidt/valuesrename
branch
from
5a90ea2 to
ef0cad8
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/valuesrename
branch
from
ef0cad8 to
d69fe94
Compare
|
Note: The first commit just moves code around, the second is the diff. |
wilhuff
reviewed
Mar 18, 2020
| @@ -385,6 +385,10 @@ function canonifyGeoPoint(geoPoint: api.LatLng): string { | |||
| return `geo(${geoPoint.latitude},${geoPoint.longitude})`; | |||
| } | |||
|
|
|||
| function canonifyReference(referenceValue: string): string { | |||
| return DocumentKey.fromName(referenceValue).toString(); | |||
There was a problem hiding this comment.
Doesn't this break existing canonical IDs?
There was a problem hiding this comment.
Yes, but the feature branch includes the schema migration.
| it('normalizes values for equality', () => { | ||
| // Each subarray compares equal to each other and false to every other value | ||
| const values: api.Value[][] = [ | ||
| [{ integerValue: '1' }, { integerValue: 1 }], |
There was a problem hiding this comment.
Isn't there also a case where there's a nested value object?
There was a problem hiding this comment.
If you are referring to the Int32Proto, this seems to only be used in a target's limit specification, which allows us to encode a difference between 0 and unset. I don't see any usage for this patten in api.Value.
wilhuff
approved these changes
Mar 18, 2020
| it('normalizes values for equality', () => { | ||
| // Each subarray compares equal to each other and false to every other value | ||
| const values: api.Value[][] = [ | ||
| [{ integerValue: '1' }, { integerValue: 1 }], |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #2715 for the bigger picture (and the source of values.test.ts, but it is mostly just a move of code)