Add subscribeStatus(callback) function to Document #190
Conversation
|
|
WalkthroughThe pull request introduces several changes to the Changes
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
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: 2
🧹 Outside diff range and nitpick comments (3)
Tests/Integration/IntegrationHelper.swift (1)
198-205: Consider adding error handling for edge cases.The subscription method handles the happy path well with proper memory management, but could benefit from error handling for edge cases.
func subscribeDocumentStatus() async where T == DocumentStatus { await self.doc.subscribeStatus { [weak self] event, _ in + guard let self = self else { + Logger.warning("EventCollector was deallocated during status subscription") + return + } guard let status = (event as? StatusChangedEvent)?.value.status else { + Logger.warning("Received unexpected event type: \(type(of: event))") return } self?.add(event: status) } }Tests/Integration/DocumentIntegrationTests.swift (1)
474-541: LGTM! Consider extracting sleep duration to a constant.The test comprehensively covers various document status events with clear scenarios and verifications.
Consider extracting the sleep duration to a constant for better maintainability:
+ private let syncWaitDuration: UInt64 = 500_000_000 // 500msSources/Document/Document.swift (1)
198-205: Consider enhancing the documentationWhile the implementation is correct, the documentation could be more detailed about when status changes occur (e.g., during attach, detach, or removal operations) to help API consumers better understand when to expect callbacks.
Add more detailed documentation:
/** * `subscribeStatus` registers a callback to subscribe to events on the document. * The callback will be called when the document status changes. + * + * Status changes occur in the following scenarios: + * - When the document is attached to a client + * - When the document is detached from a client + * - When the document is removed */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- Sources/Core/Client.swift (3 hunks)
- Sources/Document/Document.swift (3 hunks)
- Tests/Integration/DocumentIntegrationTests.swift (1 hunks)
- Tests/Integration/IntegrationHelper.swift (1 hunks)
🔇 Additional comments (9)
Tests/Integration/IntegrationHelper.swift (2)
157-166: LGTM! Well-structured class declaration.The
EventCollectorclass is well-designed with appropriate use of generics and clear property declarations.
157-206: Verify comprehensive test coverage for DocumentStatus events.While the
EventCollectorprovides good infrastructure for testing status changes, ensure that all possible document status transitions are covered in the actual test cases.✅ Verification successful
Test coverage for DocumentStatus events is comprehensive
The test suite includes dedicated test cases that verify document status transitions:
test_subscribe_document_status_changed_event: Tests basic status change events during document attachmenttest_document_status_changes_to_detached_when_deactivating: Verifies status changes during client deactivation- Tests cover both manual and automatic synchronization modes
- Multiple clients (c1, c2) are used to verify status changes across different scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases using EventCollector with DocumentStatus rg -l "EventCollector.*DocumentStatus" Tests/Length of output: 96
Script:
#!/bin/bash # Let's examine the actual test cases in DocumentIntegrationTests.swift rg "func test.*Document.*Status" -A 10 Tests/Integration/DocumentIntegrationTests.swiftLength of output: 89
Script:
#!/bin/bash # Let's try a broader search to find all test cases in DocumentIntegrationTests.swift rg "func test" -B 2 -A 15 Tests/Integration/DocumentIntegrationTests.swift # Also search for any DocumentStatus usage in the test file rg "DocumentStatus" -B 2 -A 5 Tests/Integration/DocumentIntegrationTests.swiftLength of output: 6511
Tests/Integration/DocumentIntegrationTests.swift (1)
543-576: Verify the intended behavior of document status changes during deactivation.The test documents current behavior where document status changes to
detachedduring deactivation. However, the note suggests this might change in the future.Consider:
- Adding a TODO with a ticket/issue reference to track the potential behavior change.
- Renaming the test if the behavior changes to maintain clarity.
Let's verify if there are any ongoing discussions or plans about this behavior:
✅ Verification successful
Document status transition to
detachedduring deactivation is the intended behaviorBased on the documentation in
DocumentStatusenum,detachedstatus explicitly means "the document is not attached to the client." This aligns perfectly with the deactivation scenario, as a deactivated client cannot maintain attached documents. The current implementation is consistent with the documented behavior, and there's no evidence of planned changes in the codebase.Consider:
- Removing the "NOTE" comment about potential future changes since it creates unnecessary uncertainty
- The test name accurately reflects the current and intended behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related discussions about document status behavior rg -i "document.*status.*deactivat" --type md rg -i "document.*detached.*deactivat" --type mdLength of output: 97
Script:
#!/bin/bash # Let's search for related code patterns and documentation about document status transitions # Search for document status enum definition ast-grep --pattern 'enum DocumentStatus { $$$ }' # Search for deactivate implementations that might affect document status ast-grep --pattern 'func deactivate($$$) { $$$ }' # Search for any comments or documentation about document status behavior rg -A 3 -B 3 "document.*status" --type swiftLength of output: 3326
Script:
#!/bin/bash # Let's search for the DocumentStatus enum definition and its usage rg -p "enum DocumentStatus" -A 10 --type swift # Search for any issues or TODOs related to document status rg -i "todo.*document.*status" --type swift rg -i "fixme.*document.*status" --type swift # Search for deactivate implementation in Client class ast-grep --pattern 'class Client { $$$ func deactivate($$$) { $$$ } $$$ }'Length of output: 665
Sources/Document/Document.swift (3)
91-91: LGTM: Property declaration follows established patternsThe
statusSubscribeCallbackproperty declaration is consistent with other subscription callback properties in the class.
651-652: LGTM: Event handling implementation is consistentThe status change event handling follows the established pattern in the publish method, with proper null-safe callback invocation.
198-205: Verify status change callback triggersLet's verify that the status callback is properly triggered in all status change scenarios.
✅ Verification successful
Let's gather more specific information about how status changes are handled and tested.
Status change callback implementation is properly integrated
The verification shows that the status callback system is well-implemented:
- Status changes are properly handled in
applyStatusmethod- The callback is triggered in the event publishing system when status changes occur
- Status changes are covered by integration tests
- The callback is invoked for all status change scenarios (attached, detached, removed)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all places where document status is changed echo "Searching for status change points..." rg -l "applyStatus|status\s*=" --type swift # Search for test coverage of status changes echo "Checking test coverage for status changes..." rg -l "test.*status.*change" --type swift Tests/Length of output: 429
Script:
#!/bin/bash # Check how status is modified in Document.swift echo "Status modifications in Document.swift:" rg "status\s*=" -B 2 -A 2 Sources/Document/Document.swift # Check how applyStatus is implemented echo -e "\nStatus application implementation:" rg "applyStatus" -B 2 -A 2 Sources/Document/Document.swift # Check test coverage details echo -e "\nStatus change test implementations:" rg "test.*status.*change" -B 2 -A 2 Tests/Integration/DocumentIntegrationTests.swift # Check if statusSubscribeCallback is called echo -e "\nCallback invocations:" rg "statusSubscribeCallback" -B 2 -A 2 Sources/Document/Document.swiftLength of output: 2723
Sources/Core/Client.swift (3)
218-222: LGTM! Improved document status handling during deactivation.The changes ensure that document status is properly updated to 'detached' before cleanup, which maintains consistency between the document state and the client's attachment map.
340-340: LGTM! Consolidated detachment logic.
685-694: LGTM! Well-implemented internal helper method.The new
detachInternalmethod follows good practices:
- Proper guard clause for safety
- Clear separation of concerns
- Focused responsibility for document detachment cleanup
| func asyncStream() -> AsyncStream<T> { | ||
| return AsyncStream<T> { continuation in | ||
| for value in self.values { | ||
| continuation.yield(value) | ||
| } | ||
| continuation.finish() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing AsyncStream to support real-time updates.
The current implementation creates a one-shot stream that only yields existing values. Consider enhancing it to support real-time updates by maintaining a continuation that yields new values as they arrive.
func asyncStream() -> AsyncStream<T> {
- return AsyncStream<T> { continuation in
+ return AsyncStream<T> { [weak self] continuation in
+ guard let self = self else { return }
for value in self.values {
continuation.yield(value)
}
- continuation.finish()
+ // Store continuation to yield future values
+ self.continuation = continuation
}
}Committable suggestion was skipped due to low confidence.
| func verifyNthValue(at nth: Int, isEqualTo targetValue: T) async { | ||
| if nth > self.values.count { | ||
| XCTFail("Expected \(nth)th value: \(targetValue), but the stream ended before reaching the value") | ||
| } | ||
|
|
||
| var counter = 0 | ||
| for await value in self.asyncStream() { | ||
| counter += 1 | ||
|
|
||
| if counter == nth { | ||
| print("\(nth)th, value: \(value)") | ||
| XCTAssertTrue(value == targetValue, "Expected \(nth)th value: \(targetValue), actual value: \(value)") | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix boundary check and enhance error handling.
Several improvements can be made to the verification method:
- The boundary check should be
>=instead of>to handle the exact nth case - The print statement should use proper logging
- Error messages could be more descriptive
func verifyNthValue(at nth: Int, isEqualTo targetValue: T) async {
- if nth > self.values.count {
+ if nth >= self.values.count {
- XCTFail("Expected \(nth)th value: \(targetValue), but the stream ended before reaching the value")
+ XCTFail("Expected \(nth)th value: \(targetValue), but only received \(self.values.count) values")
}
var counter = 0
for await value in self.asyncStream() {
counter += 1
if counter == nth {
- print("\(nth)th, value: \(value)")
+ Logger.debug("Verifying \(nth)th value: expected=\(targetValue), actual=\(value)")
XCTAssertTrue(value == targetValue, "Expected \(nth)th value: \(targetValue), actual value: \(value)")
break
}
}
}Committable suggestion was skipped due to low confidence.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
==========================================
+ Coverage 45.10% 45.85% +0.74%
==========================================
Files 105 105
Lines 19321 26778 +7457
==========================================
+ Hits 8715 12278 +3563
- Misses 10606 14500 +3894 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
Sources/Util/Codable+Extension.swift (1)
39-39: Consider documenting nil-safety behavior.The current implementation silently returns nil on encoding failures. Consider adding a documentation comment to make this behavior explicit.
var toJSONString: String? { let encoder = JSONEncoder() encoder.outputFormatting = .sortedKeys guard let data = try? encoder.encode(self) else { return nil } + // Returns nil if the data cannot be converted to a UTF-8 string return String(data: data, encoding: .utf8) }Sources/Util/Dictionary+Extension.swift (1)
82-85: LGTM! Consider making the empty string case more explicit.The change to use
String(data:encoding:)improves clarity by making the UTF-8 encoding explicit. This aligns well with similar changes across the codebase for consistent string handling.Consider making the empty string fallback case more explicit about the failure reason:
if let result = try? JSONSerialization.data(withJSONObject: $0, options: [.fragmentsAllowed, .withoutEscapingSlashes, .sortedKeys]), let stringValue = String(data: result, encoding: .utf8) { return stringValue } else { - return "" + return "" // Return empty string if JSON serialization or UTF-8 encoding fails }Tests/Integration/IntegrationHelper.swift (1)
197-204: Enhance error handling and guard condition clarity.While the implementation is generally good, consider these improvements:
- Make the guard condition more explicit about what constitutes an invalid event
- Consider collecting error cases for debugging purposes
func subscribeDocumentStatus() async where T == DocumentStatus { await self.doc.subscribeStatus { [weak self] event, _ in - guard let status = (event as? StatusChangedEvent)?.value.status else { + guard let statusEvent = event as? StatusChangedEvent else { + Logger.debug("Unexpected event type: \(type(of: event))") return } + guard let status = statusEvent.value.status else { + Logger.debug("Status event contains no status value") + return + } self?.add(event: status) } }Sources/Document/CRDT/CRDTTree.swift (1)
490-493: LGTM! Consider enhancing error logging.The changes improve JSON serialization by properly handling UTF-8 encoding and providing a fallback for failures.
Consider adding debug logging when serialization fails to help with troubleshooting:
if let data = try? JSONSerialization.data(withJSONObject: toDictionary, options: [.sortedKeys]), let jsonString = String(data: data, encoding: .utf8) { return jsonString +} else { + Logger.debug("Failed to serialize CRDTTreeNode to JSON") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- Sources/API/Converter.swift (1 hunks)
- Sources/Document/CRDT/CRDTTree.swift (1 hunks)
- Sources/Util/Codable+Extension.swift (1 hunks)
- Sources/Util/Dictionary+Extension.swift (1 hunks)
- Tests/Integration/IntegrationHelper.swift (1 hunks)
- Tests/Unit/Document/CRDT/PrimitiveTests.swift (1 hunks)
🔇 Additional comments (6)
Sources/Util/Codable+Extension.swift (1)
39-39: LGTM! More idiomatic Swift code.The change to use
String(data:encoding:)aligns better with Swift conventions for Data-to-String conversion.Tests/Unit/Document/CRDT/PrimitiveTests.swift (1)
93-93: LGTM! Good improvement in API consistency.The change from
String(decoding:as:)toString(data:encoding:)is a positive improvement that aligns with similar updates across the codebase, making the string conversion methods more consistent and idiomatic Swift.Tests/Integration/IntegrationHelper.swift (3)
157-166: LGTM! Well-structured class declaration.The class is well-designed with appropriate use of generics and clear property definitions.
172-179: Skip review - issue already addressed.A previous review already suggested enhancing AsyncStream to support real-time updates.
181-195: Skip review - issue already addressed.A previous review already suggested fixing the boundary check and enhancing error handling.
Sources/API/Converter.swift (1)
37-40: Improved error handling for invalid UTF-8 data.The change enhances string decoding robustness by explicitly handling UTF-8 decoding failures. This is a good improvement that prevents silent failures.
Let's verify how the error is handled in calling code:
✅ Verification successful
Based on the search results, I can now generate a final response:
The improved error handling is safe to implement
The code changes enhance error handling without disrupting existing patterns:
- All
valueFromcalls in tests (PrimitiveTests.swift) usetryand handle type-specific cases- The new
YorkieError.unimplementedaligns with existing error handling patterns in the codebase- No direct callers will be broken as they already use try-catch patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of valueFrom to ensure proper error handling # Test: Search for direct calls to valueFrom rg -A 5 'valueFrom\(' # Test: Search for catch blocks that might handle YorkieError.unimplemented rg -A 5 'catch.*YorkieError\.unimplemented'Length of output: 5801
Script:
#!/bin/bash # Let's search for error handling patterns around valueFrom calls # Search for try/catch blocks around valueFrom rg -B 2 -A 3 'try.*valueFrom.*\{.*catch' -U # Search for all YorkieError cases to understand error handling patterns rg 'case\s+YorkieError\.' -A 2 # Search for any error handling in test files rg 'XCTAssertThrowsError.*valueFrom' -A 3Length of output: 3137
| func add(event: T) { | ||
| self.values.append(event) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding thread safety to event collection.
The add method might be accessed from multiple threads during testing. Consider using a concurrent data structure or synchronization mechanism.
+ private let queue = DispatchQueue(label: "com.yorkie.eventcollector", attributes: .concurrent)
+ private var _values: [T] = []
+ var values: [T] {
+ queue.sync { _values }
+ }
func add(event: T) {
- self.values.append(event)
+ queue.async(flags: .barrier) {
+ self._values.append(event)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func add(event: T) { | |
| self.values.append(event) | |
| } | |
| private let queue = DispatchQueue(label: "com.yorkie.eventcollector", attributes: .concurrent) | |
| private var _values: [T] = [] | |
| var values: [T] { | |
| queue.sync { _values } | |
| } | |
| func add(event: T) { | |
| queue.async(flags: .barrier) { | |
| self._values.append(event) | |
| } | |
| } |
hiddenviewer
force-pushed
the
doc-subscribe-status
branch
from
31f22f5
to
ea5e526
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
Tests/Integration/IntegrationHelper.swift (1)
170-172: Add documentation to explain the class and initializer purpose.Consider adding documentation comments to explain:
- The purpose of the EventCollector class
- The relationship with the Document parameter
- Usage examples in tests
+/// A thread-safe collector for document events that supports verification in tests. +/// Usage: +/// ```swift +/// let collector = EventCollector<DocumentStatus>(doc: document) +/// await collector.subscribeDocumentStatus() +/// await collector.verifyNthValue(at: 1, isEqualTo: .attached) +/// ``` class EventCollector<T: Equatable> { // ... + /// Creates a new event collector for the specified document. + /// - Parameter doc: The document whose events will be collected. init(doc: Document) { self.doc = doc }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Tests/Integration/IntegrationHelper.swift (1 hunks)
🔇 Additional comments (2)
Tests/Integration/IntegrationHelper.swift (2)
157-168: LGTM! Well-structured class definition with thread-safe properties.The implementation demonstrates good practices with proper encapsulation and thread safety using a concurrent queue.
180-187: Verify stream behavior with concurrent event additions.The current implementation might miss events added after stream creation. Consider adding tests to verify the behavior with concurrent event additions.
| func verifyNthValue(at nth: Int, isEqualTo targetValue: T) async { | ||
| if nth > self.values.count { | ||
| XCTFail("Expected \(nth)th value: \(targetValue), but only received \(self.values.count) values") | ||
| } | ||
|
|
||
| var counter = 0 | ||
| for await value in self.asyncStream() { | ||
| counter += 1 | ||
|
|
||
| if counter == nth { | ||
| XCTAssertTrue(value == targetValue, "Expected \(nth)th value: \(targetValue), actual value: \(value)") | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize verification method with early return.
Consider adding an early return when the target value is found to avoid unnecessary iterations.
func verifyNthValue(at nth: Int, isEqualTo targetValue: T) async {
if nth > self.values.count {
XCTFail("Expected \(nth)th value: \(targetValue), but only received \(self.values.count) values")
+ return
}
var counter = 0
for await value in self.asyncStream() {
counter += 1
if counter == nth {
XCTAssertTrue(value == targetValue, "Expected \(nth)th value: \(targetValue), actual value: \(value)")
- break
+ return
}
}
+ XCTFail("Stream ended before finding \(nth)th value")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func verifyNthValue(at nth: Int, isEqualTo targetValue: T) async { | |
| if nth > self.values.count { | |
| XCTFail("Expected \(nth)th value: \(targetValue), but only received \(self.values.count) values") | |
| } | |
| var counter = 0 | |
| for await value in self.asyncStream() { | |
| counter += 1 | |
| if counter == nth { | |
| XCTAssertTrue(value == targetValue, "Expected \(nth)th value: \(targetValue), actual value: \(value)") | |
| break | |
| } | |
| } | |
| } | |
| func verifyNthValue(at nth: Int, isEqualTo targetValue: T) async { | |
| if nth > self.values.count { | |
| XCTFail("Expected \(nth)th value: \(targetValue), but only received \(self.values.count) values") | |
| return | |
| } | |
| var counter = 0 | |
| for await value in self.asyncStream() { | |
| counter += 1 | |
| if counter == nth { | |
| XCTAssertTrue(value == targetValue, "Expected \(nth)th value: \(targetValue), actual value: \(value)") | |
| return | |
| } | |
| } | |
| XCTFail("Stream ended before finding \(nth)th value") | |
| } |
hiddenviewer
force-pushed
the
doc-subscribe-status
branch
from
ea5e526
to
19a8ea5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
Tests/Integration/IntegrationHelper.swift (1)
170-173: Add documentation for public initializer.Consider adding documentation comments to explain the purpose and usage of this public initializer.
+/// Creates an event collector for the specified document. +/// - Parameter doc: The document whose events will be collected. init(doc: Document) { self.doc = doc }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Tests/Integration/IntegrationHelper.swift (1 hunks)
🔇 Additional comments (1)
Tests/Integration/IntegrationHelper.swift (1)
157-169: LGTM! Well-structured class with thread-safe implementation.The class is properly designed with:
- Thread-safe concurrent queue implementation
- Clear separation of concerns between private storage and public interface
- Appropriate use of computed properties
| func subscribeDocumentStatus() async where T == DocumentStatus { | ||
| await self.doc.subscribeStatus { [weak self] event, _ in | ||
| guard let status = (event as? StatusChangedEvent)?.value.status else { | ||
| return | ||
| } | ||
| self?.add(event: status) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for subscription failures.
Consider handling potential errors from the subscription to prevent silent failures in tests.
- func subscribeDocumentStatus() async where T == DocumentStatus {
+ func subscribeDocumentStatus() async throws where T == DocumentStatus {
- await self.doc.subscribeStatus { [weak self] event, _ in
+ try await self.doc.subscribeStatus { [weak self] event, error in
+ if let error = error {
+ XCTFail("Status subscription failed: \(error)")
+ return
+ }
guard let status = (event as? StatusChangedEvent)?.value.status else {
return
}
self?.add(event: status)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func subscribeDocumentStatus() async where T == DocumentStatus { | |
| await self.doc.subscribeStatus { [weak self] event, _ in | |
| guard let status = (event as? StatusChangedEvent)?.value.status else { | |
| return | |
| } | |
| self?.add(event: status) | |
| } | |
| } | |
| func subscribeDocumentStatus() async throws where T == DocumentStatus { | |
| try await self.doc.subscribeStatus { [weak self] event, error in | |
| if let error = error { | |
| XCTFail("Status subscription failed: \(error)") | |
| return | |
| } | |
| guard let status = (event as? StatusChangedEvent)?.value.status else { | |
| return | |
| } | |
| self?.add(event: status) | |
| } | |
| } |
| func verifyNthValue(at nth: Int, isEqualTo targetValue: T) async { | ||
| if nth > self.values.count { | ||
| XCTFail("Expected \(nth)th value: \(targetValue), but only received \(self.values.count) values") | ||
| return | ||
| } | ||
|
|
||
| var counter = 0 | ||
| for await value in self.asyncStream() { | ||
| counter += 1 | ||
|
|
||
| if counter == nth { | ||
| XCTAssertTrue(value == targetValue, "Expected \(nth)th value: \(targetValue), actual value: \(value)") | ||
| return | ||
| } | ||
| } | ||
|
|
||
| XCTFail("Stream ended before finding \(nth)th value") | ||
| } |
There was a problem hiding this comment.
Improve boundary check for better error handling.
The boundary check should use >= instead of > to handle the exact nth case consistently.
- if nth > self.values.count {
+ if nth >= self.values.count {
XCTFail("Expected \(nth)th value: \(targetValue), but only received \(self.values.count) values")
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func verifyNthValue(at nth: Int, isEqualTo targetValue: T) async { | |
| if nth > self.values.count { | |
| XCTFail("Expected \(nth)th value: \(targetValue), but only received \(self.values.count) values") | |
| return | |
| } | |
| var counter = 0 | |
| for await value in self.asyncStream() { | |
| counter += 1 | |
| if counter == nth { | |
| XCTAssertTrue(value == targetValue, "Expected \(nth)th value: \(targetValue), actual value: \(value)") | |
| return | |
| } | |
| } | |
| XCTFail("Stream ended before finding \(nth)th value") | |
| } | |
| func verifyNthValue(at nth: Int, isEqualTo targetValue: T) async { | |
| if nth >= self.values.count { | |
| XCTFail("Expected \(nth)th value: \(targetValue), but only received \(self.values.count) values") | |
| return | |
| } | |
| var counter = 0 | |
| for await value in self.asyncStream() { | |
| counter += 1 | |
| if counter == nth { | |
| XCTAssertTrue(value == targetValue, "Expected \(nth)th value: \(targetValue), actual value: \(value)") | |
| return | |
| } | |
| } | |
| XCTFail("Stream ended before finding \(nth)th value") | |
| } |
What this PR does / why we need it:
doc.subscribe('status', callback)function yorkie-js-sdk#828Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests