Skip to content

Move Client.Watch inside Client.Attach#803

Merged
hackerwins merged 5 commits intomainfrom
hide_watch_in_attach_in_client
Apr 16, 2024
Merged

Move Client.Watch inside Client.Attach#803
hackerwins merged 5 commits intomainfrom
hide_watch_in_attach_in_client

Conversation

@krapie
Copy link
Member

@krapie krapie commented Feb 28, 2024

What this PR does / why we need it:

Move Client.Watch() inside Client.Attach() to hide it from external interface.
This PR also introduces Client.Subscribe() which performs similar functionality with other SDKs.

This PR is recreated from #593

Which issue(s) this PR fixes:

Address #584

Special notes for your reviewer:

I prefer keeping this PR size as small as possible to wrap up this task on time.

Does this PR introduce a user-facing change?:

Go SDK user will use `Client.Subscribe()` instead of `Client.Watch()`, just like other SDKs.

Additional documentation:


Checklist:

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

---------

Co-authored-by: karockai <karockai@gmail.com>
@krapie krapie requested review from hackerwins and removed request for hackerwins February 28, 2024 10:40
@krapie krapie self-assigned this Feb 28, 2024
@krapie krapie added the cleanup 🧹 Paying off technical debt label Feb 28, 2024
@codecov
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 39 lines in your changes are missing coverage. Please review.

Project coverage is 50.81%. Comparing base (0987cbc) to head (f053e38).
Report is 10 commits behind head on main.

Files Patch % Lines
client/client.go 0.00% 39 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #803      +/-   ##
==========================================
+ Coverage   50.71%   50.81%   +0.09%     
==========================================
  Files          70       70              
  Lines       10213    10255      +42     
==========================================
+ Hits         5180     5211      +31     
- Misses       4512     4516       +4     
- Partials      521      528       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@krapie krapie force-pushed the hide_watch_in_attach_in_client branch 2 times, most recently from 5ab07b5 to 1fe58eb Compare February 28, 2024 13:30
@krapie krapie force-pushed the hide_watch_in_attach_in_client branch from 1fe58eb to 7b2dd71 Compare February 28, 2024 13:50
@krapie krapie marked this pull request as ready for review March 5, 2024 10:27
@krapie krapie requested review from chacha912 and hackerwins March 5, 2024 10:27
@hackerwins
Copy link
Member

Thank you for your contribution. In JS SDK, there is also a setting for SyncMode as shown in the link below. How about applying them together?

yorkie-team/yorkie-js-sdk#772 (review)

@krapie
Copy link
Member Author

krapie commented Apr 13, 2024

Is the syncMode feature big? Like I mentioned, I want to keep this PR small.

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 simple questions.

@krapie krapie requested a review from hackerwins April 16, 2024 09:49
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 5ce2aa3 into main Apr 16, 2024
@hackerwins hackerwins deleted the hide_watch_in_attach_in_client branch April 16, 2024 10:41
@hackerwins
Copy link
Member

@krapie, @karockai Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup 🧹 Paying off technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants