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 panic from crdt counter #598

Merged

Conversation

MoonGyu1
Copy link
Contributor

@MoonGyu1 MoonGyu1 commented Aug 4, 2023

What this PR does / why we need it:

Remove the panic methods in crdt/counter.go

  • Before, there were panic methods to reveal errors in development environment, but because it risks stopping the server in production, so those panics were replaced with error methods.
  • As the panic methods in counter were replaced, the related test codes also were modified.

Which issue(s) this PR fixes:

Related to #497 (crdt/counter)

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 Aug 4, 2023

CLA assistant check
All committers have signed the CLA.

@MoonGyu1 MoonGyu1 closed this Aug 4, 2023
@MoonGyu1 MoonGyu1 reopened this Aug 4, 2023
@krapie krapie requested a review from hackerwins August 4, 2023 09:21
@krapie krapie added the cleanup 🧹 Paying off technical debt label Aug 4, 2023
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #598 (e0e37cb) into main (c6ed71b) will decrease coverage by 0.29%.
Report is 3 commits behind head on main.
The diff coverage is 47.76%.

@@            Coverage Diff             @@
##             main     #598      +/-   ##
==========================================
- Coverage   51.47%   51.19%   -0.29%     
==========================================
  Files          66       67       +1     
  Lines        6962     7053      +91     
==========================================
+ Hits         3584     3611      +27     
- Misses       2904     2960      +56     
- Partials      474      482       +8     
Files Changed Coverage Δ
api/converter/from_bytes.go 51.18% <0.00%> (-1.25%) ⬇️
api/converter/to_bytes.go 62.94% <0.00%> (-1.14%) ⬇️
api/converter/to_pb.go 55.24% <50.00%> (-0.21%) ⬇️
api/converter/from_pb.go 49.00% <55.55%> (-0.19%) ⬇️
pkg/document/crdt/counter.go 54.08% <58.13%> (+0.99%) ⬆️

... and 5 files with indirect coverage changes

@hackerwins hackerwins requested a review from krapie August 6, 2023 02:32
krapie
krapie previously approved these changes Aug 6, 2023
Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution 👍🏼

I left a small comment below.

pkg/document/crdt/counter.go Outdated Show resolved Hide resolved
@krapie krapie self-requested a review August 6, 2023 04:20
@krapie krapie dismissed their stale review August 6, 2023 04:21

I misclicked approve button :(

Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

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

Follow the comment above 😅

To prevent stopping the server in production environment,
the panic methods were replaced with error methods.

Modify test codes for counter

As the panic methods in counter were replaced,
the related test codes also were modified.

Add error check for Increase method of counter

Add error check for Increase method of counter
@MoonGyu1 MoonGyu1 force-pushed the remove-panic-from-crdt-counter branch from fd8f68d to e6200ba Compare August 7, 2023 01:36
@MoonGyu1 MoonGyu1 requested a review from krapie August 7, 2023 02:04
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. 👍

@hackerwins hackerwins merged commit b8c2a5c into yorkie-team:main Aug 7, 2023
1 check passed
hackerwins pushed a commit that referenced this pull request Aug 7, 2023
To prevent stopping the server in the production environment,
the panic methods were replaced with error methods.

Modify test codes for counter

As the panic methods in the counter were replaced,
the related test codes also were modified.
Wu22e pushed a commit to Wu22e/yorkie that referenced this pull request Sep 3, 2023
To prevent stopping the server in the production environment,
the panic methods were replaced with error methods.

Modify test codes for counter

As the panic methods in the counter were replaced,
the related test codes also were modified.
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.

4 participants