Skip to content

Comments

doc(examples): add missing examples in ro_example_test.go#124

Merged
samber merged 1 commit intomainfrom
doc/examples-mergewith-mergemap-combinelatest
Oct 19, 2025
Merged

doc(examples): add missing examples in ro_example_test.go#124
samber merged 1 commit intomainfrom
doc/examples-mergewith-mergemap-combinelatest

Conversation

@samber
Copy link
Owner

@samber samber commented Oct 19, 2025

No description provided.

@claude
Copy link

claude bot commented Oct 19, 2025

📝 PR Review Summary

This PR adds comprehensive examples for combining operators and includes some important bug fixes. Overall, it's a valuable contribution that improves the library's documentation and fixes existing issues.

Positive Changes

  1. Bug Fix in CombineLatestWith4 (operator_combining.go:590)

    • Corrected atomic comparison from < 4 to < 5
    • This fixes a potential issue where the operator wouldn't properly detect completion with 5 input observables
  2. API Simplification (operator_transformations_test.go)

    • Updated MapTo[int, int] to MapTo[int], removing redundant type parameters
    • This improves type inference and makes the API more ergonomic
  3. Comprehensive Documentation (ro_example_test.go)

    • Added extensive examples for all combining operators: MergeWith, CombineLatestWith, ZipWith, etc.
    • Covers both success and error scenarios
    • Examples follow established code patterns

⚠️ Areas of Concern

  1. Timing-Dependent Test Reliability

    • Many examples use hardcoded time.Sleep() calls (25ms, 50ms, 75ms, etc.)
    • These could be flaky in CI environments or under different load conditions
    • Consider using more deterministic synchronization patterns
  2. Inconsistent Cleanup Patterns

    • Mix of subscription.Wait() and defer subscription.Unsubscribe()
    • Could confuse users about best practices for resource management
    • Recommendation: Standardize on defer subscription.Unsubscribe() for examples
  3. Breaking API Change

    • The MapTo signature change is a breaking change
    • Should be documented in release notes for user migration

🔧 Recommendations

  1. For timing-sensitive examples, consider using channels or other synchronization primitives instead of time.Sleep()
  2. Standardize subscription cleanup patterns across all examples
  3. Add a note in the PR description or release notes about the MapTo API change
  4. Consider adding a comment explaining why certain examples use time.Sleep() vs Wait()

🎯 Overall Assessment

Approval Status: ✅ Approve with suggestions

The PR provides significant value by:

  • Fixing a legitimate bug in the combining operator
  • Improving the API ergonomics
  • Adding much-needed comprehensive documentation

The concerns are relatively minor and don't block the PR approval. The timing issues in examples are acceptable for demonstration purposes, though future improvements could make them more robust.

Great work on adding these comprehensive examples! 🚀

@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.17%. Comparing base (288b8bd) to head (cf240cf).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #124      +/-   ##
==========================================
+ Coverage   62.28%   69.17%   +6.88%     
==========================================
  Files          76       76              
  Lines        9485     9485              
==========================================
+ Hits         5908     6561     +653     
+ Misses       3492     2839     -653     
  Partials       85       85              
Flag Coverage Δ
unittests 69.17% <ø> (+6.88%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@samber samber force-pushed the doc/examples-mergewith-mergemap-combinelatest branch from d95e6f6 to cf240cf Compare October 19, 2025 21:30
@claude
Copy link

claude bot commented Oct 19, 2025

Pull Request Review

Summary

This PR adds extensive example functions to ro_example_test.go demonstrating usage of the reactive observable library. The examples cover creation, combination, filtering, transformation, error handling, and subject types.

✅ Strengths

  • Comprehensive examples covering most/all public APIs
  • Consistent naming pattern (ExampleMethod_name)
  • Good coverage of success and error cases
  • Examples demonstrate realistic usage patterns
  • Proper use of defer subscription.Unsubscribe()

⚠️ Issues Found

  1. Inconsistent Output Comments: Some examples show incorrect output:

    • ExampleNewUnicastSubject_overflow:5094 - Comment says '456 and 789 logged' but output only shows 'Completed'
    • Several examples may have mismatched expected output
  2. Large File Size: At 5106 lines, the file is becoming difficult to navigate. Consider splitting into multiple files by category (creation, filtering, transformation, etc.)

  3. Missing Examples: No examples found for some operators like WindowWhen (marked as TODO in tests)

  4. Test Placeholder: TestOperatorTransformationMapErrI:161 is empty with just a TODO comment

Recommendations

  1. Verify all example outputs match actual behavior
  2. Consider organizing examples into multiple files for better maintainability
  3. Implement the missing MapErrI test or remove the placeholder
  4. Add examples for any operators that are missing them

The examples are valuable for users learning the library, but ensuring accuracy will improve developer experience.

@claude
Copy link

claude bot commented Oct 19, 2025

Additional Issue Found

Incorrect Example Output

In ExampleNewUnicastSubject_overflow (around line 5094):

  • The comment states: "subject.Subscribe(PrintObserverint) // 456 and 789 logged"
  • But the expected output only shows: "// Output: // Completed"

This discrepancy suggests the example output does not match the actual behavior. The UnicastSubject with buffer size 2 should emit the last 2 values (456, 789) when a subscriber subscribes after completion.

Please verify and correct this example to ensure accuracy of the documentation.

@samber samber added documentation Improvements, corrections, or additions to documentation: markdowns, readme, docs/ website.... core Changes affecting the core logic or main components of the project, in root/internal/testing pkg labels Oct 19, 2025
@samber samber merged commit 16c7db0 into main Oct 19, 2025
22 checks passed
@samber samber deleted the doc/examples-mergewith-mergemap-combinelatest branch October 19, 2025 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Changes affecting the core logic or main components of the project, in root/internal/testing pkg documentation Improvements, corrections, or additions to documentation: markdowns, readme, docs/ website....

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants