feat(data-structures/unstable): align RollingCounter with other data structures, add at() and toArray()#7102
feat(data-structures/unstable): align RollingCounter with other data structures, add at() and toArray()#7102tomas-zijdemans wants to merge 3 commits intodenoland:mainfrom
at() and toArray()#7102Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7102 +/- ##
==========================================
- Coverage 94.61% 94.61% -0.01%
==========================================
Files 634 634
Lines 51801 51818 +17
Branches 9329 9336 +7
==========================================
+ Hits 49011 49027 +16
Misses 2216 2216
- Partials 574 575 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bartlomieju
left a comment
There was a problem hiding this comment.
Overall solid PR. The new at() and toArray() methods are well-implemented, and the alignment changes (Iterable, readonly snapshot, @experimental tags, complexity table, Symbol.toStringTag as property) are all consistent with how Deque and other data structures work in this repo. Tests are thorough.
Two things:
1. Empty segments should throw RangeError, not TypeError
An empty array is the correct type — it's just the wrong length. The constructor throws RangeError for the equivalent constraint (segmentCount < 1), so from() should do the same for consistency. The other two TypeError changes (null/non-object snapshot, non-array segments) are correct.
2. PR scope vs title
The title says "add at() and toArray()" but the PR also changes the snapshot interface to readonly, adds implements Iterable<number>, switches Symbol.toStringTag to a readonly property, adds @experimental to all members, adds the complexity table, and reworks from() validation. The description documents all of this, but the commit message will be misleading in git log. Worth updating the title/commit message to something broader like "align RollingCounter with other data structures, add at() and toArray()".
RollingCounter.at() and toArray()at() and toArray()
…structures, add `at()` and `toArray()` Made-with: Cursor
4b6e0b7 to
7a2bea5
Compare
|
Thanks,
|
This PR is mostly about aligning RollingCounter with Deque, IndexedHeap, MultiMap and other data-structures.