Skip to content
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

Remove Select operation from Text #589

Merged

Conversation

joonhyukchoi
Copy link
Contributor

@joonhyukchoi joonhyukchoi commented Jul 26, 2023

What this PR does / why we need it:

Recently we moved presence to document from client and the role of Text.select has become replaceable. So we decided to remove Select operation from Text.

Which issue(s) this PR fixes:

Address #583

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

@CLAassistant
Copy link

CLAassistant commented Jul 26, 2023

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #589 (a87c6e2) into main (1b557be) will decrease coverage by 0.06%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

❗ Current head a87c6e2 differs from pull request most recent head ec7ee0a. Consider uploading reports for the commit ec7ee0a to get more accurate results

@@            Coverage Diff             @@
##             main     #589      +/-   ##
==========================================
- Coverage   51.11%   51.06%   -0.06%     
==========================================
  Files          66       66              
  Lines        7008     6970      -38     
==========================================
- Hits         3582     3559      -23     
+ Misses       2952     2941      -11     
+ Partials      474      470       -4     
Files Changed Coverage Δ
api/converter/from_pb.go 47.85% <ø> (-0.46%) ⬇️
api/converter/to_pb.go 53.77% <ø> (-1.36%) ⬇️
pkg/document/crdt/rga_tree_split.go 70.73% <ø> (+1.13%) ⬆️
pkg/document/crdt/text.go 59.18% <ø> (+1.34%) ⬆️

@joonhyukchoi joonhyukchoi marked this pull request as draft July 26, 2023 15:06
@joonhyukchoi joonhyukchoi marked this pull request as ready for review July 26, 2023 15:12
@hackerwins hackerwins mentioned this pull request Jul 27, 2023
3 tasks
@hackerwins hackerwins self-requested a review July 27, 2023 01:46
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

I left a comment.
#583 (comment)

@hackerwins
Copy link
Member

The migration appears to be challenging due to operations being stored in a binary format. To accomplish this task, we would need to retrieve operations from DB, unmarshal them using the previous version of the protobuf message and then marshal them again with the new version before storing them back in DB.

To avoid the migration process, how about retaining the orders in the protobuf message as they are?

@hackerwins hackerwins self-requested a review August 1, 2023 02:06
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@hackerwins hackerwins merged commit f571e72 into yorkie-team:main Aug 1, 2023
Wu22e pushed a commit to Wu22e/yorkie that referenced this pull request Sep 3, 2023
* Remove Select operation from Text

* Retaining the order of protobuf message
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.

3 participants