fix: cluster size of 0 leads to infinite loop #397
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark Results
Performance regressions of 30% or more should be investigated, unless they were anticipated. Smaller regressions may be due to normal variability, as we don't use dedicated CI infrastructure. |
fResult
force-pushed
the
fix/cluster-size-errors
branch
from
260b096 to
42ca8be
Compare
aleclarson
requested changes
May 8, 2025
There was a problem hiding this comment.
Hey @fResult, good to see more contributions from you 👍
I'll get this merged after my review is addressed
| @@ -13,6 +15,14 @@ export function cluster<T, Size extends number = 2>( | |||
| array: readonly T[], | |||
| size: Size = 2 as Size, | |||
| ): Cluster<T, Size>[] { | |||
| if (size <= 0 || Number.isNaN(size)) { | |||
There was a problem hiding this comment.
The NaN check is unnecessary, since the loop will exit when size is NaN.
| throw new IllegalSizeError(`Size must be 1 or more, the size is ${size}`) | ||
| } | ||
|
|
||
| if (size === Number.POSITIVE_INFINITY) { |
There was a problem hiding this comment.
Same here. The loop will exit just fine when size is infinite.
| @@ -13,6 +15,14 @@ export function cluster<T, Size extends number = 2>( | |||
| array: readonly T[], | |||
| size: Size = 2 as Size, | |||
| ): Cluster<T, Size>[] { | |||
| if (size <= 0 || Number.isNaN(size)) { | |||
| throw new IllegalSizeError(`Size must be 1 or more, the size is ${size}`) | |||
There was a problem hiding this comment.
I think returning [] when size <= 0 would be better. It's more concise, makes logical sense, and works fine with the Cluster<T, Size>[] return type.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes infinite loop bug when cluster() size is 0.
I chose error throwing over returning an input array due to
Cluster<T, Size>type constraints.Additional test cases were added to verify handling of various edge cases, including negative and non-integer sizes.
Related issue, if any:
#392
For any code change,
Does this PR introduce a breaking change?
No
Bundle impact
src/array/cluster.tsFootnotes
Function size includes the
importdependencies of the function. ↩