-
Notifications
You must be signed in to change notification settings - Fork 26
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
Adding sync state service to get latest row version based on given row ids #3128
Conversation
1be1007
to
7f84325
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.
Sorry for missing the meeting on this, probably some of these were discussed there.
api_types.ts
Outdated
* | ||
* TODO(ebo): unhide this | ||
*/ | ||
readonly syncState: SyncStateService; |
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.
Can we maybe call this syncStateService
? Isn't sync
an object that already represents "sync state"?
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 guess it might be clearer if we include service
in the name? wdyt @erickoledadevrel
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 like adding "Service"
* | ||
* TODO(ebo): unhide this | ||
*/ | ||
getLatestRowVersions(rowIds: string[]): Promise<{[rowId: string]: string}>; |
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.
We call things items and item ids in most places, but we use "rows" and row ids" here, how come the difference?
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.
We felt like it would be better to standardize on "rows". Here's the accompanying change, it's pretty small: #3129
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.
38343a1
to
c45f2da
Compare
c45f2da
to
07b401b
Compare
Discussion: https://coda.io/d/go-brainwriteups_dQYLHXsHZWS/Pack-SDK-Get-item-versions-interface_suFrdPVO?utm_source=slack#_luPjCMl1
Follow up: update
item ids
torow ids
for ingestion related variablesPTAL: @patrick-codaio @erickoledadevrel @jonathan-codaio