Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Chunk #2252

Merged
merged 3 commits into from Jul 28, 2015
Merged

Chunk #2252

merged 3 commits into from Jul 28, 2015

Conversation

megawac
Copy link
Collaborator

@megawac megawac commented Jul 28, 2015

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

Petr Archakov added 2 commits July 28, 2015 10:30
@megawac
Add _.chunk to Array functions.
4d95823 
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
@megawac
chunk(): set default parts to 1
b221fe7
@jdalton
Copy link
Contributor

jdalton commented Jul 28, 2015

I'll align lodash to this implementation.

@michaelficarra
Copy link
Collaborator

There are no tests for a default count. I'm still against the idea of having a default count.

michaelficarra
michaelficarra reviewed Jul 28, 2015
View reviewed changes
underscore.js
// Split an **array** into several arrays containing **count** or less elements
// of initial array
_.chunk = function(array, count) {
count = count != null ? count : 1;
Copy link
Collaborator

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;.

Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@jridgewell
Copy link
Collaborator

There are no tests for a default count.

Line 549?

What's your argument against a default count?

@michaelficarra
Copy link
Collaborator

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.

jridgewell
jridgewell reviewed Jul 28, 2015
View reviewed changes
underscore.js
// of initial array
_.chunk = function(array, count) {
count = count != null ? count : 1;
if (count < 1) return [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an [[]]?

Copy link
Contributor

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 🐰

Copy link
Collaborator

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. 😛

Copy link
Contributor

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. 😛

Copy link
Collaborator

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.

@megawac
Copy link
Collaborator Author

megawac commented Jul 28, 2015

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.

I sort of agree, however I don't want to deviate from lodash too much

@jdalton
Copy link
Contributor

jdalton commented Jul 28, 2015

The default could be 0 too (no strong opinion).
Look to built-in methods with similar params, they have default values.

@megawac
Copy link
Collaborator Author

megawac commented Jul 28, 2015

I'm fine with 0 or 1

@jdalton
Copy link
Contributor

jdalton commented Jul 28, 2015

The one weak win for having it be 1 would be when it's used as an iteratee in _.map (assuming call guards are in place) but that's a pretty narrow case.

I went with 1 before because that's the smallest param for a meaningful result.

@michaelficarra
Copy link
Collaborator

A default of 0 will just return [] unconditionally. That's probably the least surprising default. By the way @jdalton are you coming to the TC39 meeting? There's like 20 MSFT employees here and you're not one of them.

@jdalton
Copy link
Contributor

jdalton commented Jul 28, 2015

@michaelficarra

By the way @jdalton are you coming to the TC39 meeting? There's like 20 MSFT employees here and you're not one of them.

Imma be at the dinner tonight. On the agenda items I'm 👍 on trimLeft and trimRight (no name change) and I'm experimenting with the more thorough/robust RegExp.escape implementation being discussed (it escapes leading numbers and things) in lodash as _.escapeRegExp.

@jdalton
Copy link
Contributor

jdalton commented Jul 28, 2015

@megawac

I'm fine with 0 or 1

I'm down with 0 being a default. It means there's little use for it as a _.map iteratee which means no need to add a guard for it either (keeps it simple).

@megawac megawac force-pushed the chunk branch 2 times, most recently from 4900f96 to d413815 Compare July 28, 2015 20:00
@megawac
Copy link
Collaborator Author

megawac commented Jul 28, 2015

Right on, amended.

jridgewell added a commit that referenced this pull request Jul 28, 2015
@jridgewell
Merge pull request #2252 from megawac/chunk
d845c0b 
Chunk
@jridgewell jridgewell merged commit d845c0b into jashkenas:master Jul 28, 2015
@megawac megawac deleted the chunk branch July 28, 2015 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement fixed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@megawac @jdalton @michaelficarra @jridgewell @jashkenas