-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chunk #2252
Chunk #2252
Conversation
fix _.chunk function fix _.chunk function Do not return original array if chunkin in more then array size parts Do not return original array if chunkin in more then array size parts spaces clean up
I'll align lodash to this implementation. |
There are no tests for a default count. I'm still against the idea of having a default count. |
// Split an **array** into several arrays containing **count** or less elements | ||
// of initial array | ||
_.chunk = function(array, count) { | ||
count = count != null ? count : 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there's two consequences, as in all conditional expressions, don't negate the test. Should be count = count == null ? 1 : count
. Or we could avoid an unnecessary assignment and use if (count == null) count = 1;
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the if
statement, though the reversed ternary would be good too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
What's your argument against a default |
Ah, I didn't see that one. My argument against a default count is that I don't feel any particular count is "special" or should be assumed or will be used more frequently than others. |
// of initial array | ||
_.chunk = function(array, count) { | ||
count = count != null ? count : 1; | ||
if (count < 1) return []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an [[]]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to the rabbit hole 🐰
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess leave as it. 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the correct answer is no.
I sort of agree, however I don't want to deviate from |
The default could be |
I'm fine with |
The one weak win for having it be I went with |
A default of 0 will just return |
Imma be at the dinner tonight. On the agenda items I'm 👍 on |
I'm down with |
4900f96
to
d413815
Compare
Right on, amended. |
Resolves #1919 pull request (squash, adapted fork of that branch)
Biggest deviation from pr above (and lodash) is
chunkSize < 1
returns an array of 0 chunks (empty array)Closes a bunch of issues #2130 #1891 #2249 #696 #998 #714 (others perhaps?)
Ping underdash/underdash#7 gkz/prelude-ls#81