-
Notifications
You must be signed in to change notification settings - Fork 53
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
refactor: Generate field ids using a sequence #2339
refactor: Generate field ids using a sequence #2339
Conversation
e7ef6b4
to
9287d95
Compare
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.
Overall looks great. I just have few comments.
|
||
// FieldIDSequenceKey is used to key the sequence used to generate field ids. | ||
// | ||
// The sequence is specific to each collection root. Multiple collection of the same root |
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.
question: Can you remind me what the root is? Do you mean the root schema?
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.
Nevermind. I saw the answer bellow 😅
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!
This makes it consistent with the rest of our keys.
This makes it consistent with the rest of our keys.
9287d95
to
7ba6239
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2339 +/- ##
===========================================
+ Coverage 74.82% 74.83% +0.01%
===========================================
Files 257 257
Lines 25314 25349 +35
===========================================
+ Hits 18940 18968 +28
- Misses 5083 5090 +7
Partials 1291 1291
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Relevant issue(s)
Resolves #2333
Description
Generates field ids using a sequence instead of their index in the array.
This is required for allowing field deletion and schema branching as otherwise those two will alter the position and thus id of fields. For example, previously if two schema branches were to add a new field each, the new fields would share the same id and mess the datastore cache up in interesting ways.
WARNING
This PR is currently branched off of #2336 - please only review commits from 'Simplify isNew evaluation' onwards here.