Rework FieldMask to use a SortedSet of FieldPaths #1405
Conversation
rsgowman
requested review from
gsoltis,
mikelehen,
schmidt-sebastian,
var-const,
wilhuff and
zxu123
as code owners
|
One slight difference: For the ts port, I used FieldMask.fromArray rather than .fromSet, due to the difficulty in creating a SortedSet out of a list of values. Alternatively, we could add another ctor to SortedSet that takes an initializer list (similar to es6's Set). |
packages/testing/src/api/index.ts
Outdated
| // efficiency is less important than responsiveness. | ||
| 'grpc.initial_reconnect_backoff_ms': 100, | ||
| 'grpc.max_reconnect_backoff_ms': 100 | ||
| } |
There was a problem hiding this comment.
why is this showing up here?
cc/ @ryanpbrewster who made this change elsewhere and @Feiyang1 who had it show up in his PR too....
There was a problem hiding this comment.
This was merged as part of #1403, not sure why it's showing up here...
There was a problem hiding this comment.
I think I know what's going on. I rebased the master yesterday after the release and the PRs are submitted before the rebase. Rebasing the PR should solve the problem.
There was a problem hiding this comment.
It's an unfortunate side effect with the release branch - master branch needs to rebase on the release branch for the version bump/release tag to show up in the right order.
There was a problem hiding this comment.
Hrm. Rebasing master sounds problematic. Can't we just do a normal merge of the release branch into master?
There was a problem hiding this comment.
Please don't rebase master. Everyone who has the repo checked out will have to perform a fix. For long-lived branches like master and our release branches, the only thing we can do is merge (and resolve conflicts).
There was a problem hiding this comment.
The problem with normal merge is that the version bump/release tag will appear at the HEAD, and it would be misleading to see commits that are not released appear before the release tag.
There was a problem hiding this comment.
Agreed it's not ideal and potentially dangerous. I'm open to change the process if we found better solution.
There was a problem hiding this comment.
I think just doing a normal (non-squash) merge would result in a sensible history.
Else, a squash merge with a clear description ("Merge release branch changes and version bump back into master.") would probably be sufficient.
Ultimately, we should be using the release branch and/or git tags to figure out what was actually in a given release.
There was a problem hiding this comment.
Right--the release branch should reflect the state of the release. The master branch reflects ongoing development that may have happened while that branch was coming about. If we merge the release branch back to master it will naturally show the history of when things happened.
Master is preparing for the next release.
Rather than an array. Port of firebase/firebase-android-sdk#137
rsgowman
force-pushed
the
rsgowman/field_mask_set
branch
from
cc364a1 to
f3ec700
Compare
Rather than an array.
Port of firebase/firebase-android-sdk#137