FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: added changes for seq_id by vkalta · Pull Request #29 · contentstack/contentstack-ios · GitHub

feat: added changes for seq_id - #29

Draft
vkalta wants to merge 7 commits into
developmentfrom
feat/seq_id_changes
Draft

feat: added changes for seq_id#29
vkalta wants to merge 7 commits into
developmentfrom
feat/seq_id_changes

Conversation

vkalta commented Feb 8, 2024
edited
Loading

Copy link
Copy Markdown

This PR is a enhancement task with respect to the previous sync API implementation by replacing sync token with seq id.

vkalta requested a review from a team as a code owner February 8, 2024 10:06

ishaileshmishra left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@vkalta, https://github.com/contentstack/contentstack-ios/blob/a2f87854f0c0a2232d653b39a52f5126a70e6ea1/ContentstackInternal/BSONObjectIdGenerator.m

seq_id generation using bason lib should be inside the persistence sdk not here. this is part of persistence

vkalta commented Feb 13, 2024

Copy link
Copy Markdown
Author

@vkalta, https://github.com/contentstack/contentstack-ios/blob/a2f87854f0c0a2232d653b39a52f5126a70e6ea1/ContentstackInternal/BSONObjectIdGenerator.m

seq_id generation using bason lib should be inside the persistence sdk not here. this is part of persistence

By the design currently the seq id, sync token and pagination is currently being updated inside the contentstack-ios project. This is why it was deemed to be added here and not in persistence package. Please let me know your thoughts on it.

Thanks

Comment thread Contentstack/Stack.h Outdated

@param completionBlock called synchronization is done.
*/
- (void)syncSeqIdInit:(void (^)(SyncStack * BUILT_NULLABLE_P syncStack, NSError * BUILT_NULLABLE_P error))completionBlock;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The function should be named initSeqSync or initialSequenceSync.

Reasoning: Including "id" in the function name may cause confusion for developers as it doesn't serve any purpose in this context. This function doesn't require any input from the developer.

@vkalta

ishaileshmishra left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Approved

thanks @vkalta

Copy link
Copy Markdown

@vkalta , approved but before merging the PR make sure to add below changes :

  • Update the seqIdSync to suggested name in the comment
  • Checks are failing , please check with @aravindbuilt

thank you !

netrajpatel changed the base branch from next to development September 24, 2024 06:43
reeshika-h marked this pull request as draft October 4, 2024 05:12
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL