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

Add _.chunk to Array functions. #1919

Closed
wants to merge 11 commits into from
Closed

Add _.chunk to Array functions. #1919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e917a18
Add _.decompose to Array functions.
Oct 7, 2014
db7f405
Merge remote-tracking branch 'upstream/master'
Nov 4, 2014
750f22c
Add _.chunk to Array functions.
Nov 5, 2014
dbc7375
merge chunk to master
Nov 5, 2014
e3035ba
fix _.chunk function
Nov 6, 2014
63652f6
fix _.chunk function
Nov 6, 2014
a59a6f9
remove _.decompose from array functions
Nov 7, 2014
7d4034c
Do not return original array if chunkin in more then array size parts
Nov 17, 2014
ce3a67a
Do not return original array if chunkin in more then array size parts
Nov 17, 2014
0c6d140
spaces clean up
Jan 20, 2015
185abf6
chunk(): set default parts to 1
Apr 13, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 18 additions & 1 deletion test/arrays.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,5 +418,22 @@
deepEqual(_.range(12, 7, -2), [12, 10, 8], 'range with three arguments a & b & c, a > b, c < 0 generates an array of elements a,a-c,a-2c and ends with the number not less than b');
deepEqual(_.range(0, -10, -1), [0, -1, -2, -3, -4, -5, -6, -7, -8, -9], 'final example in the Python docs');
});


test('chunk', function() {
deepEqual(_.chunk([], 2), [], 'chunk for empty array returns an empty array');

deepEqual(_.chunk([1, 2, 3], 0), [1, 2, 3], 'chunk into parts of 0 elements returns original array');

deepEqual(_.chunk([1, 2, 3], 1), [1, 2, 3], 'chunk into parts of 1 elements returns original array');
deepEqual(_.chunk([1, 2, 3]), [1, 2, 3], 'chunk into parts of 1 elements is default value and returns original array');

deepEqual(_.chunk([1, 2, 3], -1), [1, 2, 3], 'chunk into parts of negative amount of elements returns an empty array');

deepEqual(_.chunk([1, 2, 3], 3), [[1, 2, 3]], 'chunk into parts of current array length elements returns the original array');
deepEqual(_.chunk([1, 2, 3], 5), [[1, 2, 3]], 'chunk into parts of more then current array length elements returns the original array');

deepEqual(_.chunk([10, 20, 30, 40, 50, 60, 70], 2), [[10, 20], [30, 40], [50, 60], [70]], 'chunk into parts of less then current array length elements');
deepEqual(_.chunk([10, 20, 30, 40, 50, 60, 70], 3), [[10, 20, 30], [40, 50, 60], [70]], 'chunk into parts of less then current array length elements');
});

}());
17 changes: 17 additions & 0 deletions underscore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,23 @@
return range;
};

// Split an **array** into several arrays containing **count** or less elements
// of initial array
_.chunk = function(array, count) {
count = 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.

To support the count = undefined case and fix the count = 1 case, we should default to 0 here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the default = 1 makes sense. However do we want to mirror lodash
which makes anything less than 1 mean 1 chunk size?

E.g. _.chunk([1,2,3], 0) => [[1], [2], [3]]

On Tue, Jul 28, 2015 at 11:32 AM, Justin Ridgewell <notifications@github.com

wrote:

In underscore.js
#1919 (comment):

@@ -647,6 +647,23 @@
return range;
};

  • // Split an array into several arrays containing count or less elements
  • // of initial array
  • _.chunk = function(array, count) {
  • count = count || 1;

To support the count = undefined case and fix the count = 1 case, we
should default to 0 here.

—
Reply to this email directly or view it on GitHub
https://github.com/jashkenas/underscore/pull/1919/files#r35661334.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that behaviour sort of makes sense so that chunk always returns an
array of arrays

On Tue, Jul 28, 2015 at 12:18 PM, Graeme Yeates megawac@gmail.com wrote:

I think the default = 1 makes sense. However do we want to mirror lodash
which makes anything less than 1 mean 1 chunk size?

E.g. _.chunk([1,2,3], 0) => [[1], [2], [3]]

On Tue, Jul 28, 2015 at 11:32 AM, Justin Ridgewell <
notifications@github.com> wrote:

In underscore.js
#1919 (comment):

@@ -647,6 +647,23 @@
return range;
};

  • // Split an array into several arrays containing count or less elements
  • // of initial array
  • _.chunk = function(array, count) {
  • count = count || 1;

To support the count = undefined case and fix the count = 1 case, we
should default to 0 here.

—
Reply to this email directly or view it on GitHub
https://github.com/jashkenas/underscore/pull/1919/files#r35661334.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only other reasonable option I see is to return an empty array for
chunk < 1

On Tue, Jul 28, 2015 at 12:19 PM, Graeme Yeates megawac@gmail.com wrote:

I think that behaviour sort of makes sense so that chunk always returns an
array of arrays

On Tue, Jul 28, 2015 at 12:18 PM, Graeme Yeates megawac@gmail.com wrote:

I think the default = 1 makes sense. However do we want to mirror lodash
which makes anything less than 1 mean 1 chunk size?

E.g. _.chunk([1,2,3], 0) => [[1], [2], [3]]

On Tue, Jul 28, 2015 at 11:32 AM, Justin Ridgewell <
notifications@github.com> wrote:

In underscore.js
#1919 (comment)
:

@@ -647,6 +647,23 @@
return range;
};

  • // Split an array into several arrays containing count or less elements
  • // of initial array
  • _.chunk = function(array, count) {
  • count = count || 1;

To support the count = undefined case and fix the count = 1 case, we
should default to 0 here.

—
Reply to this email directly or view it on GitHub
https://github.com/jashkenas/underscore/pull/1919/files#r35661334.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's fine with me, I was just coding the the current tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

/cc @jdalton

With v4 coming, nows the time to talk about these details

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the current behavior is correct. If your chunk is less than 1 there is no chunk and a shallow clone is returned.
Think of it like if you had a pie and you cut it into 0 slices you still have a whole pie.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A shallow clone isn't returned in lodash. It returns the array in chunks of 1. I think returning a shallow clone would be even worse as the user would have to worry about arrays of arrays and arrays of any.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm actually leaning with @megawac here. We're not cutting the array into 3 slices, we're gathering into groups of 3. And groups of 0 don't have anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm ok cool.


if (count <= 1) return array.slice();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Direct call to slice should instead use slice helper.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, definitely broken. It should return an array of singletons.

Should be count < 1 to fix.


var result = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, I wonder if it's faster to compute the length new Array(Math.ceil(array.length / count)) - feel free not to bother with it

var i = 0, length = array.length;

while (i < length) {
result.push(slice.call(array, i, i += count));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably write line 655-659 as

var i = 0, length = array && array.length, j = 0;
while (i < length && count >= 0) {
     if (i % count) result[j++] = [];
     result[j].push(array[i]);
}

(untested). I would be curious to see some benchmarks


return result;
};

// Function (ahem) Functions
// ------------------

Expand Down