Unify error throwing methods #878
Conversation
This commit separates the error throwing and logging functionalities in logger.fatal. The logger now solely handles logging messages, while errors are thrown independently using YorkieError.
This commit standardizes the naming conventions for errors starting with 'Err'
This commit replaces all instances of throw new Error with YorkieError to provide more specific and consistent error handling throughout the codebase.
WalkthroughThe recent updates to the Yorkie SDK significantly enhance error handling across various components by implementing structured error reporting through the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Outside diff range, codebase verification and nitpick comments (1)
src/document/document.ts (1)
1881-1884: Ensure proper error handling forredofunction callsThe search results indicate that the
redofunction throwsYorkieError, but there is no evidence of try-catch blocks or explicit error handling around calls toredo()in the implementation files. This could lead to unhandled exceptions in the application.
- Ensure that all calls to
redo()are wrapped in try-catch blocks or have appropriate error handling to manageYorkieError.Analysis chain
LGTM! But verify the function usage in the codebase.
The code changes are approved.
However, ensure that all function calls to
redohandle the newYorkieErrorappropriately.
The
redofunction is used extensively in the codebase, particularly in test files, which is a good indication that error handling is being tested. However, we need to ensure that all instances whereredois used in the actual implementation (not just tests) handle theYorkieErrorappropriately. Let's search forredocalls in implementation files and check if they are within try-catch blocks or have appropriate error handling.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `redo` handle the new `YorkieError` appropriately. # Test: Search for the function usage. Expect: Appropriate handling of `YorkieError`. rg --type ts -A 5 $'redo'Length of output: 26840
Script:
#!/bin/bash # Search for redo calls in implementation files and check for error handling. rg --type ts 'redo()' -A 5 | grep -E 'try|catch|YorkieError'Length of output: 235
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (28)
- src/api/converter.ts (6 hunks)
- src/document/crdt/element_rht.ts (3 hunks)
- src/document/crdt/rga_tree_list.ts (4 hunks)
- src/document/crdt/rga_tree_split.ts (4 hunks)
- src/document/crdt/root.ts (2 hunks)
- src/document/crdt/tree.ts (5 hunks)
- src/document/document.ts (5 hunks)
- src/document/json/counter.ts (3 hunks)
- src/document/json/text.ts (11 hunks)
- src/document/json/tree.ts (24 hunks)
- src/document/operation/add_operation.ts (2 hunks)
- src/document/operation/edit_operation.ts (2 hunks)
- src/document/operation/increase_operation.ts (2 hunks)
- src/document/operation/move_operation.ts (2 hunks)
- src/document/operation/operation.ts (2 hunks)
- src/document/operation/remove_operation.ts (2 hunks)
- src/document/operation/set_operation.ts (2 hunks)
- src/document/operation/style_operation.ts (2 hunks)
- src/document/operation/tree_edit_operation.ts (2 hunks)
- src/document/operation/tree_style_operation.ts (2 hunks)
- src/util/error.ts (3 hunks)
- src/util/index_tree.ts (22 hunks)
- src/util/logger.ts (1 hunks)
- src/util/observable.ts (2 hunks)
- src/util/splay_tree.ts (2 hunks)
- test/integration/client_test.ts (3 hunks)
- test/integration/object_test.ts (2 hunks)
- test/integration/primitive_test.ts (2 hunks)
Additional context used
Biome
test/integration/primitive_test.ts
[error] 41-41: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
GitHub Check: build (18.x)
src/document/crdt/element_rht.ts
[failure] 118-118:
Variable nameERROR_MESSAGEmust match one of the following formats: camelCase, PascalCase
[failure] 148-148:
Variable nameERROR_MESSAGEmust match one of the following formats: camelCase, PascalCasesrc/document/crdt/root.ts
[failure] 138-138:
Variable nameERROR_MESSAGEmust match one of the following formats: camelCase, PascalCasesrc/document/crdt/rga_tree_list.ts
[failure] 184-184:
Variable nameERROR_MESSAGEmust match one of the following formats: camelCase, PascalCase
[failure] 238-238:
Variable nameERROR_MESSAGEmust match one of the following formats: camelCase, PascalCase
[failure] 245-245:
Variable nameERROR_MESSAGEmust match one of the following formats: camelCase, PascalCase
[failure] 294-294:
Variable nameERROR_MESSAGEmust match one of the following formats: camelCase, PascalCasesrc/document/crdt/rga_tree_split.ts
[failure] 638-638:
Variable nameERROR_MESSAGEmust match one of the following formats: camelCase, PascalCase
[failure] 797-797:
Variable nameERROR_MESSAGEmust match one of the following formats: camelCase, PascalCase
[failure] 851-851:
Variable nameERROR_MESSAGEmust match one of the following formats: camelCase, PascalCase
Additional comments not posted (79)
test/integration/primitive_test.ts (1)
6-6: New import statement is appropriate.The addition of
CodeandYorkieErroris necessary for the new error handling approach.src/util/error.ts (2)
30-31: New error codes improve error handling.The addition of
ErrInvalidType,ErrDummy,ErrOperationNotReady, andErrOperationNotPermittedenhances the expressiveness of error handling.Also applies to: 33-34, 51-52, 54-55
67-67: SimplifiedtoStringmethod improves clarity.The updated
toStringmethod focuses on the error code and message, reducing verbosity.src/util/logger.ts (1)
Line range hint
55-55:
Removing error throwing fromfatalmethod is appropriate.The removal aligns with the goal of separating logging and error throwing responsibilities.
src/document/json/counter.ts (3)
28-28: ImportingCodeandYorkieError.The import statement for
CodeandYorkieErroris appropriate for the new error handling approach.
82-84: Improved error handling inincreasemethod.The
increasemethod now throws aYorkieErrorwith a specific error code and message when the counter is not initialized. This enhances clarity and consistency in error handling.
109-109: Improved error handling intoJSForTestmethod.The
toJSForTestmethod now throws aYorkieErrorwith a specific error code and message when the counter is not ready. This improves the robustness and clarity of error handling.src/document/operation/add_operation.ts (3)
26-26: ImportingCodeandYorkieError.The import statement for
CodeandYorkieErroris appropriate for the new error handling approach.
64-66: Improved error handling when parent object is not found.The
executemethod now throws aYorkieErrorwith a specific error code and message when the parent object is not found. This enhances clarity and consistency in error handling.
69-71: Improved error handling for invalid parent object type.The
executemethod now throws aYorkieErrorwith a specific error code and message when the parent object is not an instance ofCRDTArray. This improves the robustness and clarity of error handling.src/document/operation/move_operation.ts (3)
25-25: ImportingCodeandYorkieError.The import statement for
CodeandYorkieErroris appropriate for the new error handling approach.
68-70: Improved error handling when parent object is not found.The
executemethod now throws aYorkieErrorwith a specific error code and message when the parent object is not found. This enhances clarity and consistency in error handling.
73-75: Improved error handling for invalid parent object type.The
executemethod now throws aYorkieErrorwith a specific error code and message when the parent object is not an instance ofCRDTArray. This improves the robustness and clarity of error handling.src/document/operation/increase_operation.ts (2)
66-68: Good practice: Centralized error message and structured error handlingCentralizing the error message definition and throwing a
YorkieErrorwith a specific error code improves readability and maintainability.
71-73: Good practice: Centralized error message and structured error handlingCentralizing the error message definition and throwing a
YorkieErrorwith a specific error code improves readability and maintainability.src/document/operation/remove_operation.ts (2)
72-74: Good practice: Centralized error message and structured error handlingCentralizing the error message definition and throwing a
YorkieErrorwith a specific error code improves readability and maintainability.
77-79: Good practice: Centralized error message and structured error handlingCentralizing the error message definition and throwing a
YorkieErrorwith a specific error code improves readability and maintainability.src/document/operation/style_operation.ts (2)
81-83: Good practice: Centralized error message and structured error handlingCentralizing the error message definition and throwing a
YorkieErrorwith a specific error code improves readability and maintainability.
86-88: Good practice: Centralized error message and structured error handlingCentralizing the error message definition and throwing a
YorkieErrorwith a specific error code improves readability and maintainability.src/document/operation/set_operation.ts (3)
28-28: Import statement for error handling approved.The inclusion of
CodeandYorkieErroris necessary for the new error handling mechanism.
70-72: Improved error handling for missing object approved.The code now logs a fatal message and throws a
YorkieErrorwhen an object is not found, providing a structured error response.
75-77: Improved error handling for invalid parent object approved.The code now logs a fatal message and throws a
YorkieErrorwhen the parent object is not an instance ofCRDTObject, providing a structured error response.src/document/operation/edit_operation.ts (3)
28-28: Import statement for error handling approved.The inclusion of
CodeandYorkieErroris necessary for the new error handling mechanism.
87-89: Improved error handling for missing parent object approved.The code now logs a fatal message and throws a
YorkieErrorwhen a parent object is not found, providing a structured error response.
92-94: Improved error handling for invalid parent object approved.The code now logs a fatal message and throws a
YorkieErrorwhen the parent object is not an instance ofCRDTText, providing a structured error response.src/util/observable.ts (3)
19-19: Import statement for error handling approved.The inclusion of
CodeandYorkieErroris necessary for the new error handling mechanism.
110-112: Improved error handling for missing observer approved.The code now logs a fatal message and throws a
YorkieErrorwhen thenextOrObserverparameter is missing, providing a structured error response.
116-118: Improved error handling for finalized observable approved.The code now logs a fatal message and throws a
YorkieErrorwhen the observable has been finalized due to a prior error, providing a structured error response.src/document/crdt/element_rht.ts (2)
118-120: Improved error handling.The change improves error handling by throwing a
YorkieErrorwith a specific error code.Tools
GitHub Check: build (18.x)
[failure] 118-118:
Variable nameERROR_MESSAGEmust match one of the following formats: camelCase, PascalCase
148-152: Improved error handling.The change improves error handling by throwing a
YorkieErrorwith a specific error code.Tools
GitHub Check: build (18.x)
[failure] 148-148:
Variable nameERROR_MESSAGEmust match one of the following formats: camelCase, PascalCasesrc/document/operation/tree_edit_operation.ts (1)
89-96: Improved error handling.The change improves error handling by throwing a
YorkieErrorwith a specific error code.src/document/operation/operation.ts (1)
214-217: Improved error handling.The change improves error handling by throwing a
YorkieErrorwith a specific error code.src/document/json/text.ts (2)
306-309: LGTM!The changes improve error handling by throwing specific errors.
321-324: LGTM!The changes improve error handling by throwing specific errors.
src/document/crdt/rga_tree_list.ts (1)
25-25: Import statement approved.The import of
CodeandYorkieErroris necessary for the new error handling approach.src/util/splay_tree.ts (1)
18-18: Import statement approved.The import of
CodeandYorkieErroris necessary for the new error handling approach.src/document/json/tree.ts (12)
46-46: Import statement approved.The import of
CodeandYorkieErroris necessary for the new error handling approach.
157-160: Error handling approved.The use of
YorkieErrorinvalidateTextNodeimproves error specificity.
179-182: Error handling approved.The use of
YorkieErrorinvalidateTreeNodesimproves error specificity.
190-193: Error handling approved.The use of
YorkieErrorinvalidateTreeNodesimproves error specificity.
260-263: Error handling approved.The use of
YorkieErroringetSizeimproves error specificity.
274-277: Error handling approved.The use of
YorkieErroringetNodeSizeimproves error specificity.
288-291: Error handling approved.The use of
YorkieErroringetIndexTreeimproves error specificity.
302-305: Error handling approved.The use of
YorkieErrorinstyleByPathimproves error specificity.
309-312: Error handling approved.The use of
YorkieErrorinstyleByPathimproves error specificity.
345-348: Error handling approved.The use of
YorkieErrorinstyleimproves error specificity.
352-355: Error handling approved.The use of
YorkieErrorinstyleimproves error specificity.
394-397: Error handling approved.The use of
YorkieErrorinremoveStyleimproves error specificity.src/document/crdt/rga_tree_split.ts (1)
29-29: Import statement forCodeandYorkieErrorlooks good.The import statement is necessary for the new error handling approach.
src/util/index_tree.ts (10)
18-18: Import statement forCodeandYorkieErrorlooks good.The import statement is necessary for the new error handling approach.
133-136: Approve the use ofYorkieErrorfor better error handling.The change improves error handling by using
YorkieErrorwith a specific error code.
310-313: Approve the use ofYorkieErrorfor better error handling.The change improves error handling by using
YorkieErrorwith a specific error code.
329-332: Approve the use ofYorkieErrorfor better error handling.The change improves error handling by using
YorkieErrorwith a specific error code.
346-349: Approve the use ofYorkieErrorfor better error handling.The change improves error handling by using
YorkieErrorwith a specific error code.
354-354: Approve the use ofYorkieErrorfor better error handling.The change improves error handling by using
YorkieErrorwith a specific error code for the child not found case.
366-369: Approve the use ofYorkieErrorfor better error handling.The change improves error handling by using
YorkieErrorwith a specific error code.
374-374: Approve the use ofYorkieErrorfor better error handling.The change improves error handling by using
YorkieErrorwith a specific error code for the child not found case.
386-389: Approve the use ofYorkieErrorfor better error handling.The change improves error handling by using
YorkieErrorwith a specific error code.
401-404: Approve the use ofYorkieErrorfor better error handling.The change improves error handling by using
YorkieErrorwith a specific error code.test/integration/client_test.ts (3)
35-35: Import statement forYorkieErrorlooks good.The import statement is necessary for the new error handling approach.
136-136: Approve the use ofYorkieErrorfor better error handling in tests.The change improves error handling in tests by using
YorkieErrorwith a specific error code.
214-214: Approve the use ofYorkieErrorfor better error handling in tests.The change improves error handling in tests by using
YorkieErrorwith a specific error code.test/integration/object_test.ts (2)
11-11: LGTM! Import statement is correct.The addition of
Codefrom@yorkie-js-sdk/src/util/erroris necessary for the new error handling approach.
66-66: LGTM! Improved error handling.The use of
YorkieErrorwith a specific error code enhances error specificity and aligns with the new error handling strategy.src/document/crdt/tree.ts (5)
46-46: LGTM! Import statement is correct.The addition of
Codefrom@yorkie-js-sdk/src/util/erroris necessary for the new error handling approach.
227-229: LGTM! Improved error handling.The use of
YorkieErrorwith a specific error code enhances error specificity and aligns with the new error handling strategy.
519-522: LGTM! Improved error handling.The use of
YorkieErrorwith a specific error code enhances error specificity and aligns with the new error handling strategy.
533-536: LGTM! Improved error handling.The use of
YorkieErrorwith a specific error code enhances error specificity and aligns with the new error handling strategy.
1228-1231: LGTM! Improved error handling.The use of
YorkieErrorwith a specific error code enhances error specificity and aligns with the new error handling strategy.src/api/converter.ts (5)
205-208: LGTM! Verify the usage of the new error code.The changes improve error specificity by using
YorkieErrorwith a specific error codeCode.ErrInvalidType.However, ensure that the new error code
Code.ErrInvalidTypeis correctly used and handled throughout the codebase.Verification successful
The usage of the error code
Code.ErrInvalidTypeis consistent and appropriate.The error code is used in both
src/api/converter.tsandsrc/document/crdt/tree.tsto ensure type safety, aligning with the intended purpose.
src/api/converter.ts: Used to handle unsupported types during conversion.src/document/crdt/tree.ts: Used to ensure the node type is correct before accessing or setting its value.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new error code `Code.ErrInvalidType`. # Test: Search for the error code usage. Expect: Consistent handling of the new error code. rg --type ts 'Code.ErrInvalidType'Length of output: 340
Script:
#!/bin/bash # Description: Gather context for the usage of `Code.ErrInvalidType` in `src/document/crdt/tree.ts`. # Search for the lines around the usage of `Code.ErrInvalidType` to understand the context. rg --type ts -A 5 -B 5 'Code.ErrInvalidType' src/document/crdt/tree.tsLength of output: 568
222-225: LGTM! Verify the usage of the new error code.The changes improve error specificity by using
YorkieErrorwith a specific error codeCode.ErrInvalidType.However, ensure that the new error code
Code.ErrInvalidTypeis correctly used and handled throughout the codebase.Verification successful
Let's inspect the usage of
Code.ErrInvalidTypeinsrc/document/crdt/tree.tsto ensure consistent error handling.
The usage of the new error code
Code.ErrInvalidTypeis consistent and appropriate throughout the codebase.
src/api/converter.ts: The error code is used with a clear message indicating an unsupported type.src/document/crdt/tree.ts: The error code is used with messages indicating issues with element node values.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new error code `Code.ErrInvalidType`. # Test: Search for the error code usage. Expect: Consistent handling of the new error code. rg --type ts 'Code.ErrInvalidType'Length of output: 340
Script:
#!/bin/bash # Search for the context around the usage of Code.ErrInvalidType in src/document/crdt/tree.ts rg --type ts -A 5 'Code.ErrInvalidType' src/document/crdt/tree.tsLength of output: 319
Line range hint
201-204:
LGTM! Verify the usage of the new error code.The changes improve error specificity by using
YorkieErrorwith a specific error codeCode.ErrUnimplemented.However, ensure that the new error code
Code.ErrUnimplementedis correctly used and handled throughout the codebase.Verification successful
The usage of the new error code
Code.ErrUnimplementedis consistent and appropriate.The error code is used throughout the codebase to throw errors with specific messages, ensuring clear and consistent error handling.
src/document/crdt/primitive.tssrc/document/crdt/tree.tssrc/document/crdt/counter.tssrc/api/converter.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new error code `Code.ErrUnimplemented`. # Test: Search for the error code usage. Expect: Consistent handling of the new error code. rg --type ts 'Code.ErrUnimplemented'Length of output: 1072
Script:
#!/bin/bash # Description: Verify how the error code `Code.ErrUnimplemented` is handled in the codebase. # Test: Search for error handling logic related to `Code.ErrUnimplemented`. rg --type ts 'Code.ErrUnimplemented' -A 5Length of output: 3797
Line range hint
56-59:
LGTM! Verify the usage of the new error code.The changes improve error specificity by using
YorkieErrorwith a specific error codeCode.ErrUnimplemented.However, ensure that the new error code
Code.ErrUnimplementedis correctly used and handled throughout the codebase.Verification successful
The usage of the new error code
Code.ErrUnimplementedis appropriate and consistent throughout the codebase.
- The error code is used with specific and clear messages in various files, which aids in debugging and maintaining the code.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new error code `Code.ErrUnimplemented`. # Test: Search for the error code usage. Expect: Consistent handling of the new error code. rg --type ts 'Code.ErrUnimplemented'Length of output: 1072
Script:
#!/bin/bash # Description: Gather context around the usage of `Code.ErrUnimplemented` in the identified files. # Search for the usage of `Code.ErrUnimplemented` in `src/document/crdt/counter.ts` with context lines. rg --type ts --context 5 'Code.ErrUnimplemented' src/document/crdt/counter.ts # Search for the usage of `Code.ErrUnimplemented` in `src/document/crdt/primitive.ts` with context lines. rg --type ts --context 5 'Code.ErrUnimplemented' src/document/crdt/primitive.ts # Search for the usage of `Code.ErrUnimplemented` in `src/document/crdt/tree.ts` with context lines. rg --type ts --context 5 'Code.ErrUnimplemented' src/document/crdt/tree.ts # Search for the usage of `Code.ErrUnimplemented` in `src/api/converter.ts` with context lines. rg --type ts --context 5 'Code.ErrUnimplemented' src/api/converter.tsLength of output: 3914
868-868: LGTM! Verify the usage of the new error code.The changes improve error specificity by using
YorkieErrorwith a specific error codeCode.ErrInvalidType.However, ensure that the new error code
Code.ErrInvalidTypeis correctly used and handled throughout the codebase.src/document/document.ts (5)
1773-1776: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
undohandle the newYorkieErrorappropriately.Verification successful
LGTM! But verify the function usage in the codebase.
The code changes are approved.
All function calls to
undohandle the newYorkieErrorappropriately, as confirmed by the extensive integration and unit tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `undo` handle the new `YorkieError` appropriately. # Test: Search for the function usage. Expect: Appropriate handling of `YorkieError`. rg --type ts -A 5 $'undo'Length of output: 40919
856-859: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
subscribehandle the newYorkieErrorappropriately.
1873-1876: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
redohandle the newYorkieErrorappropriately.
1780-1783: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
undohandle the newYorkieErrorappropriately.
663-669: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
updatehandle the newYorkieErrorappropriately.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- src/document/crdt/element_rht.ts (3 hunks)
- src/document/crdt/rga_tree_list.ts (4 hunks)
- src/document/crdt/rga_tree_split.ts (4 hunks)
- src/document/crdt/root.ts (2 hunks)
- src/document/json/counter.ts (3 hunks)
- src/document/json/text.ts (11 hunks)
- src/document/json/tree.ts (24 hunks)
- src/document/operation/add_operation.ts (2 hunks)
- src/document/operation/edit_operation.ts (2 hunks)
- src/document/operation/increase_operation.ts (2 hunks)
- src/document/operation/move_operation.ts (2 hunks)
- src/document/operation/remove_operation.ts (2 hunks)
- src/document/operation/set_operation.ts (2 hunks)
- src/document/operation/style_operation.ts (2 hunks)
- src/document/operation/tree_edit_operation.ts (2 hunks)
- src/document/operation/tree_style_operation.ts (2 hunks)
- src/util/observable.ts (2 hunks)
- src/util/splay_tree.ts (2 hunks)
- test/integration/primitive_test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (17)
- src/document/crdt/element_rht.ts
- src/document/crdt/rga_tree_list.ts
- src/document/crdt/rga_tree_split.ts
- src/document/crdt/root.ts
- src/document/json/counter.ts
- src/document/json/text.ts
- src/document/json/tree.ts
- src/document/operation/add_operation.ts
- src/document/operation/edit_operation.ts
- src/document/operation/increase_operation.ts
- src/document/operation/move_operation.ts
- src/document/operation/remove_operation.ts
- src/document/operation/set_operation.ts
- src/document/operation/style_operation.ts
- src/document/operation/tree_edit_operation.ts
- src/util/observable.ts
- src/util/splay_tree.ts
Additional context used
Biome
test/integration/primitive_test.ts
[error] 41-41: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (3)
test/integration/primitive_test.ts (2)
6-6: New import statement is necessary.The import of
CodeandYorkieErroris required for the updated error handling approach.
39-43: Improved error handling withYorkieError.The use of
YorkieErrorwith a specific error code and message enhances error handling.Tools
Biome
[error] 41-41: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/document/operation/tree_style_operation.ts (1)
111-113: Improved error handling withYorkieError.The use of
YorkieErrorwith a specific error code and message enhances error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/helper/helper.ts (2 hunks)
Additional comments not posted (2)
test/helper/helper.ts (2)
39-39: Good practice: ImportingYorkieErrorandCode.The import of
YorkieErrorandCodeenhances error handling by providing structured error reporting.
70-75: Improvement: Enhanced error handling inwaitAndVerifyNthEvent.The use of
YorkieErrorwithCode.ErrInvalidArgumentimproves the specificity and clarity of the error message. The new format is more readable and provides better context.
|
There are some points I'd like to share about the above PR:
I have revised the naming convention for error codes. Except for export enum Code {
// Ok is returned when the operation completed successfully.
Ok = 'ok',
// ErrClientNotActivated is returned when the client is not active.
ErrClientNotActivated = 'ErrClientNotActivated',
// ErrDummy is returned when the error is intentional.
ErrDummy = 'ErrDummy',
}
I have added new Error codes
fromIdx: number,
toIdx: number,
content: string,
attributes?: A,
): [number, number] | undefined {
if (!this.context || !this.text) {
const ErrorMessage = 'Text is not initialized yet';
logger.fatal(ErrorMessage);
// initialization of `Text` must be preceded ahead of the operation
throw new YorkieError(Code.ErrOperationNotReady, ErrorMessage);
}
private undo(): void {
if (this.isUpdating) {
throw new YorkieError(
Code.ErrOperationNotPermitted,
'Undo is not allowed during an update',
);
}However, I'm concerned that private undo(): void {
if (this.isUpdating) {
throw new YorkieError(
Code.ErrOperationNotPermitted,
'Undo is not allowed during an update',
);
}
I've modified the This change was made because when used with the export class YorkieError extends Error {
name = 'YorkieError';
stack?: string;
constructor(readonly code: Code, readonly message: string) {
super(message);
this.toString = (): string => `[code=${this.code}]: ${this.message}`;
}
} if (err instanceof YorkieError) {
logger.error(err.toString()); // Yorkie E: [code=ErrInvalidArgument]: message
} else {
logger.error(err);
}
I haven't made changes in the If you have any better suggestions, please let me know. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- src/document/crdt/tree.ts (5 hunks)
- src/document/document.ts (5 hunks)
- src/document/json/counter.ts (3 hunks)
- src/document/json/text.ts (11 hunks)
- src/document/json/tree.ts (24 hunks)
- src/document/operation/operation.ts (2 hunks)
- src/util/error.ts (3 hunks)
- src/util/index_tree.ts (22 hunks)
- src/util/observable.ts (2 hunks)
Files skipped from review as they are similar to previous changes (8)
- src/document/crdt/tree.ts
- src/document/document.ts
- src/document/json/text.ts
- src/document/json/tree.ts
- src/document/operation/operation.ts
- src/util/error.ts
- src/util/index_tree.ts
- src/util/observable.ts
Additional comments not posted (2)
src/document/json/counter.ts (2)
82-84: LGTM! But verify the function usage in the codebase.The code changes are approved. The error handling is now more structured and informative.
However, ensure that all function calls to
increasehandle the newYorkieErrorappropriately.
109-110: LGTM! But verify the function usage in the codebase.The code changes are approved. The error handling is now more structured and informative.
However, ensure that all function calls to
toJSForTesthandle the newYorkieErrorappropriately.
gwbaik9717
force-pushed
the
unify-error-throwing-methods
branch
from
c694382
to
ae7461b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- src/document/crdt/tree.ts (5 hunks)
- src/document/document.ts (5 hunks)
- src/document/json/counter.ts (3 hunks)
- src/document/json/text.ts (11 hunks)
- src/document/json/tree.ts (24 hunks)
- src/document/operation/operation.ts (2 hunks)
- src/util/error.ts (3 hunks)
- src/util/index_tree.ts (22 hunks)
- src/util/observable.ts (2 hunks)
Files skipped from review as they are similar to previous changes (7)
- src/document/crdt/tree.ts
- src/document/document.ts
- src/document/json/counter.ts
- src/document/json/text.ts
- src/document/operation/operation.ts
- src/util/error.ts
- src/util/observable.ts
Additional comments not posted (53)
src/document/json/tree.ts (28)
157-160: LGTM!The function correctly throws a
YorkieErrorwithCode.ErrInvalidArgumentfor an empty text node value.
179-182: LGTM!The function correctly throws a
YorkieErrorwithCode.ErrInvalidArgumentfor mixed node types.Also applies to: 190-193
260-263: LGTM!The
getSizemethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
274-277: LGTM!The
getNodeSizemethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
288-291: LGTM!The
getIndexTreemethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
302-305: LGTM!The
styleByPathmethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
309-312: LGTM!The
styleByPathmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentif the path is empty.
345-348: LGTM!The
stylemethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
352-355: LGTM!The
stylemethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentiffromIdxis greater thantoIdx.
394-397: LGTM!The
removeStylemethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
401-404: LGTM!The
removeStylemethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentiffromIdxis greater thantoIdx.
510-513: LGTM!The
editByPathmethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
516-519: LGTM!The
editByPathmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentif the path lengths are not equal.
522-525: LGTM!The
editByPathmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentif the path is empty.
549-552: LGTM!The
editBulkByPathmethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
555-558: LGTM!The
editBulkByPathmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentif the path lengths are not equal.
561-564: LGTM!The
editBulkByPathmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentif the path is empty.
583-586: LGTM!The
editmethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
589-592: LGTM!The
editmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentiffromIdxis greater thantoIdx.
616-619: LGTM!The
editBulkmethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
622-625: LGTM!The
editBulkmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentiffromIdxis greater thantoIdx.
639-642: LGTM!The
toXMLmethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
653-656: LGTM!The
toJSONmethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
668-671: LGTM!The
toJSForTestmethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
684-687: LGTM!The
toJSInfoForTestmethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
698-701: LGTM!The
getRootTreeNodemethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
712-715: LGTM!The
indexToPathmethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.
726-729: LGTM!The
pathToIndexmethod correctly throws aYorkieErrorwithCode.ErrNotInitializedif the tree is not initialized.src/util/index_tree.ts (25)
133-133: LGTM!The constructor correctly throws a
YorkieErrorwithCode.ErrRefusedif a text node has children.
307-307: LGTM!The
appendmethod correctly throws aYorkieErrorwithCode.ErrRefusedif a text node has children.
323-323: LGTM!The
prependmethod correctly throws aYorkieErrorwithCode.ErrRefusedif a text node has children.
337-337: LGTM!The
insertBeforemethod correctly throws aYorkieErrorwithCode.ErrRefusedif a text node has children.
342-342: LGTM!The
insertBeforemethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentif the reference child is not found.
354-354: LGTM!The
insertAftermethod correctly throws aYorkieErrorwithCode.ErrRefusedif a text node has children.
359-359: LGTM!The
insertAftermethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentif the reference child is not found.
371-371: LGTM!The
insertAtmethod correctly throws aYorkieErrorwithCode.ErrRefusedif a text node has children.
383-383: LGTM!The
removeChildmethod correctly throws aYorkieErrorwithCode.ErrRefusedif a text node has children.
388-388: LGTM!The
removeChildmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentif the child is not found.
439-439: LGTM!The
insertAfterInternalmethod correctly throws aYorkieErrorwithCode.ErrRefusedif a text node has children.
444-444: LGTM!The
insertAfterInternalmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentif the reference child is not found.
456-456: LGTM!The
insertAtInternalmethod correctly throws aYorkieErrorwithCode.ErrRefusedif a text node has children.
469-469: LGTM!The
findOffsetmethod correctly throws aYorkieErrorwithCode.ErrRefusedif a text node has children.
493-493: LGTM!The
findBranchOffsetmethod correctly throws aYorkieErrorwithCode.ErrRefusedif a text node has children.
581-584: LGTM!The
tokensBetweenfunction correctly throws aYorkieErrorwithCode.ErrInvalidArgumentiffromis greater thanto.
588-591: LGTM!The
tokensBetweenfunction correctly throws aYorkieErrorwithCode.ErrInvalidArgumentiffromis out of range.
595-598: LGTM!The
tokensBetweenfunction correctly throws aYorkieErrorwithCode.ErrInvalidArgumentiftois out of range.
676-679: LGTM!The
findTreePosfunction correctly throws aYorkieErrorwithCode.ErrInvalidArgumentif the index is out of range.
780-780: LGTM!The
findTextPosfunction correctly throws aYorkieErrorwithCode.ErrInvalidArgumentfor an unacceptable path.
850-850: LGTM!The
treePosToPathmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentfor an invalid tree position.
874-874: LGTM!The
treePosToPathmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentfor an invalid tree position.
898-898: LGTM!The
pathToTreePosmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentfor an unacceptable path.
907-907: LGTM!The
pathToTreePosmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentfor an unacceptable path.
916-916: LGTM!The
pathToTreePosmethod correctly throws aYorkieErrorwithCode.ErrInvalidArgumentfor an unacceptable path.
There is a problem of code being verbose with duuplicated logging and error throwing. Removing logger will temproarily resolve the issue.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
src/document/crdt/element_rht.ts (2)
118-119: Fix variable naming convention.The variable name
ErrorMessageshould follow camelCase convention.- const ErrorMessage = `fail to find ${createdAt.toIDString()}`; + const errorMessage = `fail to find ${createdAt.toIDString()}`;
147-150: Fix variable naming convention.The variable name
ErrorMessageshould follow camelCase convention.- const ErrorMessage = `fail to find ${element.getCreatedAt().toIDString()}`; + const errorMessage = `fail to find ${element.getCreatedAt().toIDString()}`;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (20)
- src/document/crdt/element_rht.ts (3 hunks)
- src/document/crdt/rga_tree_list.ts (4 hunks)
- src/document/crdt/rga_tree_split.ts (4 hunks)
- src/document/crdt/root.ts (3 hunks)
- src/document/document.ts (5 hunks)
- src/document/json/counter.ts (3 hunks)
- src/document/json/text.ts (11 hunks)
- src/document/json/tree.ts (24 hunks)
- src/document/operation/add_operation.ts (2 hunks)
- src/document/operation/edit_operation.ts (2 hunks)
- src/document/operation/increase_operation.ts (2 hunks)
- src/document/operation/move_operation.ts (2 hunks)
- src/document/operation/remove_operation.ts (2 hunks)
- src/document/operation/set_operation.ts (2 hunks)
- src/document/operation/style_operation.ts (2 hunks)
- src/document/operation/tree_edit_operation.ts (2 hunks)
- src/document/operation/tree_style_operation.ts (2 hunks)
- src/util/logger.ts (1 hunks)
- src/util/observable.ts (2 hunks)
- src/util/splay_tree.ts (2 hunks)
Files skipped from review as they are similar to previous changes (15)
- src/document/crdt/rga_tree_list.ts
- src/document/crdt/rga_tree_split.ts
- src/document/crdt/root.ts
- src/document/json/counter.ts
- src/document/json/text.ts
- src/document/json/tree.ts
- src/document/operation/add_operation.ts
- src/document/operation/edit_operation.ts
- src/document/operation/increase_operation.ts
- src/document/operation/remove_operation.ts
- src/document/operation/set_operation.ts
- src/document/operation/tree_edit_operation.ts
- src/document/operation/tree_style_operation.ts
- src/util/observable.ts
- src/util/splay_tree.ts
Additional comments not posted (11)
src/util/logger.ts (1)
94-98: LGTM! Verify the usage of thefatalmethod in the codebase.The changes align with the PR objectives to separate logging and error throwing responsibilities.
However, ensure that all calls to the
fatalmethod are reviewed to confirm they do not rely on the previous behavior of throwing an error.Verification successful
Let's expand our search criteria to ensure we capture all potential usages of the
fatalmethod.
To ensure thorough verification, let's search for the
fatalmethod usage across all file types in the codebase.
The changes to the
fatalmethod are verified.The
fatalmethod is not used elsewhere in the codebase, ensuring no dependencies rely on its previous behavior of throwing an error.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `fatal` method in the codebase. # Test: Search for the method usage. Expect: Review all occurrences to ensure they do not rely on the previous behavior of throwing an error. rg --type js -A 5 $'logger.fatal'Length of output: 32
Script:
#!/bin/bash # Description: Verify the usage of the `fatal` method in the codebase, including potential aliasing or different naming conventions. # Test: Search for the method usage. Expect: Review all occurrences to ensure they do not rely on the previous behavior of throwing an error. rg --type js -w 'fatal' -A 5Length of output: 28
Script:
#!/bin/bash # Description: Verify the usage of the `fatal` method in the codebase across all file types. # Test: Search for the method usage. Expect: Review all occurrences to ensure they do not rely on the previous behavior of throwing an error. rg -w 'fatal' -A 5Length of output: 384
src/document/operation/move_operation.ts (2)
72-73: LGTM! Verify the error handling consistency.The changes improve readability, maintainability, and consistency in error handling.
However, ensure that similar error handling patterns are consistently applied across the codebase.
68-69: LGTM! Verify the error handling consistency.The changes improve readability, maintainability, and consistency in error handling.
However, ensure that similar error handling patterns are consistently applied across the codebase.
Verification successful
Error handling consistency verified.
The
YorkieErroris consistently used across the codebase for custom error handling, ensuring readability and maintainability.
- Instances of
YorkieErrorfound in multiple files, includingsrc/document,src/util, andsrc/client.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of error handling across the codebase. # Test: Search for error handling patterns. Expect: Review similar patterns to ensure consistency. rg --type js -A 5 $'throw new YorkieError'Length of output: 43
Script:
#!/bin/bash # Description: Verify the presence and consistency of error handling across the codebase. # Search for the definition of YorkieError to understand its usage. rg 'class YorkieError' # Search for instances of YorkieError being thrown. rg 'throw new YorkieError' # Search for other error handling patterns to ensure consistency. rg 'throw new'Length of output: 26051
src/document/operation/style_operation.ts (2)
81-82: LGTM! Verify the error handling consistency.The changes improve readability, maintainability, and consistency in error handling.
However, ensure that similar error handling patterns are consistently applied across the codebase.
85-86: LGTM! Verify the error handling consistency.The changes improve readability, maintainability, and consistency in error handling.
However, ensure that similar error handling patterns are consistently applied across the codebase.
src/document/crdt/element_rht.ts (1)
20-20: LGTM! Import statement forYorkieErrorandCode.The import statement is correct and necessary for the new error handling approach.
src/document/document.ts (5)
663-663: LGTM! Unified error throwing method.The change to throw a
YorkieErrorinstead of a genericErroraligns with the objective of unifying error throwing methods.
850-853: LGTM! Improved error specificity.The change to throw a
YorkieErrorwith a specific error code and message for invalid arguments improves the specificity and consistency of error handling.
1027-1027: LGTM! Improved error specificity.The change to throw a
YorkieErrorwith a specific error code and message for invalid arguments improves the specificity and consistency of error handling.
1767-1777: LGTM! Improved error specificity.The change to throw
YorkieErrorinstances with specific error codes and messages for different error conditions in theundomethod improves the specificity and consistency of error handling.
1867-1878: LGTM! Improved error specificity.The change to throw
YorkieErrorinstances with specific error codes and messages for different error conditions in theredomethod improves the specificity and consistency of error handling.
|
Thanks to your detailed explanation, I now have a better understanding of different error throwing cases. Based on the suggestions from above, I’ve made the following changes: Firstly, I removed Secondly, I removed the // document.ts
try {
// code skipped
} catch (err) {
// removed logger.error
throw err;
}However, unlike // client.ts
.then((res) => {
// code skipped
})
.catch((err) => {
logger.error(`[DD] c:"${this.getKey()}" err:`, err);
this.handleConnectError(err);
throw err;
});Lastly, I removed the unused Please let me know, if there are any suggestions. :) |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- src/document/crdt/element_rht.ts (3 hunks)
- src/document/crdt/rga_tree_list.ts (4 hunks)
- src/document/crdt/rga_tree_split.ts (5 hunks)
- src/document/json/counter.ts (4 hunks)
- src/document/json/tree.ts (25 hunks)
- src/document/operation/add_operation.ts (3 hunks)
- src/document/operation/edit_operation.ts (3 hunks)
- src/document/operation/increase_operation.ts (2 hunks)
- src/document/operation/move_operation.ts (2 hunks)
- src/document/operation/remove_operation.ts (3 hunks)
- src/document/operation/set_operation.ts (3 hunks)
- src/document/operation/style_operation.ts (3 hunks)
- src/document/operation/tree_edit_operation.ts (3 hunks)
- src/document/operation/tree_style_operation.ts (3 hunks)
- src/util/splay_tree.ts (2 hunks)
- test/bench/tree.bench.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- test/bench/tree.bench.ts
Files skipped from review as they are similar to previous changes (14)
- src/document/crdt/element_rht.ts
- src/document/crdt/rga_tree_list.ts
- src/document/crdt/rga_tree_split.ts
- src/document/json/counter.ts
- src/document/json/tree.ts
- src/document/operation/add_operation.ts
- src/document/operation/edit_operation.ts
- src/document/operation/increase_operation.ts
- src/document/operation/move_operation.ts
- src/document/operation/set_operation.ts
- src/document/operation/style_operation.ts
- src/document/operation/tree_edit_operation.ts
- src/document/operation/tree_style_operation.ts
- src/util/splay_tree.ts
Additional comments not posted (4)
src/document/operation/remove_operation.ts (4)
32-32: Imports look good.The new imports
CodeandYorkieErrorare necessary for the improved error handling.
71-72: Improved error handling.Throwing
YorkieErrorwith a specific error code and message improves clarity and consistency.
75-76: Improved error handling.Throwing
YorkieErrorwith a specific error code and message improves clarity and consistency.
71-76: Verify consistent error handling across the codebase.Ensure that the new error handling approach is consistently applied across the codebase.
Verification successful
Verification successful: The error handling approach is consistent across the codebase.
The instances of
throw new YorkieErrorfollow a consistent pattern of using specific error codes and messages, ensuring uniform error handling.
src/util/splay_tree.tssrc/util/observable.tssrc/util/index_tree.tssrc/document/document.tssrc/document/operation/tree_style_operation.tssrc/document/operation/tree_edit_operation.tssrc/document/operation/style_operation.tssrc/document/json/text.tssrc/document/json/tree.tssrc/document/json/object.tssrc/document/operation/set_operation.tssrc/document/operation/operation.tssrc/document/operation/increase_operation.tssrc/document/operation/edit_operation.tssrc/document/json/counter.tssrc/document/operation/move_operation.tssrc/document/crdt/primitive.tssrc/document/crdt/tree.tssrc/document/operation/remove_operation.tssrc/document/crdt/rga_tree_split.tssrc/document/crdt/element_rht.tssrc/document/crdt/rga_tree_list.tssrc/document/operation/add_operation.tssrc/api/converter.tssrc/client/client.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent error handling across the codebase. # Test: Search for instances of YorkieError usage. Expect: Consistent usage of YorkieError with specific error codes and messages. rg --type-add 'ts' --type 'ts' 'throw new YorkieError'Length of output: 112
Script:
#!/bin/bash # Description: Verify consistent error handling across the codebase. # Test: Search for instances of YorkieError usage. Expect: Consistent usage of YorkieError with specific error codes and messages. rg --glob '*.ts' 'throw new YorkieError'Length of output: 12373
This commit unifies error throwing methods and removes throw error in the logger. Previously, implementation of the logger includes both logging and error throwing, which can lead to confusion and type problems. By separating these concerns, we can ensure that the logger is only responsible for logging messages, while errors are thrown explicitly using `YorkieError`.
What this PR does / why we need it?
This PR unifies error throwing methods by refactoring the
loggerto focus solely on logging without throwing errors, and useYorkieErrorto throw errors.Any background context you want to provide?
The current implementation of the
loggerincludes both logging and error throwing, which can lead to confusion and type problems. By separating these concerns, we can ensure that the logger is only responsible for logging messages, while errors are thrown explicitly usingYorkieError.What are the relevant tickets?
Fixes #696
Checklist
Summary by CodeRabbit
New Features
YorkieErrorand specific error codes.Bug Fixes