foldable: add S.takeWhile and S.dropWhile #356

gabejohnson · 2017-02-27T02:51:51Z

Closes #349

gabejohnson · 2017-02-27T02:54:40Z

I put these two functions near filter as I think they deal with Foldable, take a predicate and "reduce" the collection (in the case of Array).

davidchambers · 2017-02-27T12:11:44Z

index.js

      [Fn(a, $.Boolean), m(a), m(a)],
      Z.filterM);

+  //# takeWhile :: (Foldable f, Monoid (f a)) =>  (a -> Boolean) -> f a -> f a


Nit: There's an extra space after the =>.

davidchambers · 2017-02-27T12:20:18Z

index.js


+  //# takeWhile :: (Foldable f, Monoid (f a)) =>  (a -> Boolean) -> f a -> f a
+  //.
+  //. Returns the first elements for which the predicate returns true.


This doesn't sound quite right to me for a non-array argument. How about the following?

Discards the first inner value which does not satisfy the predicate, and all subsequent inner values.

davidchambers · 2017-02-27T12:29:48Z

index.js

+  //.
+  //. ```javascript
+  //. > S.takeWhile(c => c < 'c', ['a', 'b', 'c', 'd', 'e'])
+  //. ['a', 'b']


c => c < 'c' is slightly confusing due to c being used in two ways. n => n < 3 with [1, 2, 3, 4, 5] would be clearer. Even better, though, would be to use an example which demonstrates how this differs from filter:

> S.takeWhile(S.odd, [3, 3, 3, 7, 6, 3, 5, 4]) [3, 3, 3, 7]

davidchambers · 2017-02-27T12:33:44Z

index.js

+  //. Just(1)
+  //.
+  //. > S.takeWhile(S.K(false), S.Just(1))
+  //. Nothing


Why not use a predicate which references its argument?

> S.takeWhile(S.odd, S.Just(1)) Just(1) > S.takeWhile(S.odd, S.Just(2)) Nothing

I don't think we should include these examples, though, as they would be better expressed with filter.

I was trying to show an example of using a non-array. If we're not allowing strings, I can't think of a good example.

I think it's okay to provide array examples only.

svozza · 2017-02-27T12:28:13Z

index.js

+  function takeWhile(p, xs) {
+    if (Array.isArray(xs)) {
+      var result = [];
+      if (xs.length === 0) return result;


Probably makes more sense to create the result array after checking the length of xs.

svozza · 2017-02-27T12:35:49Z

index.js

+  function dropWhile(p, xs) {
+    if (Array.isArray(xs)) {
+      if (xs.length === 0) return [];
+      for (var i = 0; i < xs.length && p(xs[i]); i += 1);


I think idx is preferred over i.

davidchambers · 2017-02-27T12:36:43Z

index.js

+  //. > S.takeWhile(S.K(false), S.Just(1))
+  //. Nothing
+  //. ```
+  function takeWhile(p, xs) {


Let's :%s/\<p\>/pred/gc for consistency with existing code.

davidchambers · 2017-02-27T12:37:15Z

index.js

+    if (Array.isArray(xs)) {
+      var result = [];
+      if (xs.length === 0) return result;
+      for (var i = 0, x; i < xs.length && p(x = xs[i]); i += 1) result.push(x);


Let's :%s/\<i\>/idx/gc for consistency with existing code.

Interestingly, I did a search on the code we have and neither of our for loops use idx.

We don't currently have a single for loop which uses an index variable. :)

Yeah, I guess they're all in sanctuary-type-classes now.

davidchambers · 2017-02-27T12:39:09Z

index.js

+  function takeWhile(p, xs) {
+    if (Array.isArray(xs)) {
+      var result = [];
+      if (xs.length === 0) return result;


This line is unnecessary.

This is what I get for submitting a PR while on cold medicine 🤧

davidchambers · 2017-02-27T12:41:09Z

index.js

+  //. ```
+  function dropWhile(p, xs) {
+    if (Array.isArray(xs)) {
+      if (xs.length === 0) return [];


This line is unnecessary.

davidchambers · 2017-02-27T12:53:29Z

index.js

+    }
+    var drop = true;
+    function dropWhileReducer(xs, x) {
+      return drop && p(x) ? xs : (drop = false, append(x, xs));


Is this a bit clearer, do you think?

return (drop = drop && p(x)) ? xs : append(x, xs);

Not sure if a good idea, but could we borrow generators' nomenclature and go with done here? In that case this reads clearer to me:

// takeWhile return !done && p(x) ? append(x, xs) : (done = true, xs); // dropWhile return !done && p(x) ? xs : (done = true, append(x, xs));

I think @cust0dian has the clearest statement using done and the assignment in the binary expression at the end. done = true is very explicit.

I like the use of done. :)

davidchambers · 2017-02-27T12:55:27Z

test/takeWhile.js

+  eq(S.takeWhile(lt(3), [1, 2, 3, 4, 5]), [1, 2]);
+  eq(S.takeWhile(lt(4), [1, 2, 3, 4, 5]), [1, 2, 3]);
+  eq(S.takeWhile(lt(5), [1, 2, 3, 4, 5]), [1, 2, 3, 4]);
+  eq(S.takeWhile(lt(6), [1, 2, 3, 4, 5]), [1, 2, 3, 4, 5]);


Let's change these to demonstrate and assert that takeWhile is different from filter.

That's fine. I'll have to keep the tests using Maybe to maintain code coverage.

gabejohnson · 2017-03-06T14:51:25Z

This should be ready to go.

davidchambers · 2017-03-06T16:05:48Z

index.js

+           idx < xs.length && pred(x = xs[idx]);
+           idx += 1) result.push(x);
+      return result;
+    }


Let's move this to a separate function:

function Array$takeWhile(pred, xs) { ... } function takeWhile(pred, xs) { if (Array.isArray(xs)) return Array$takeWhile(pred, xs); ... } S.takeWhile = def(...);

Could we use xs.slice(idx) rather than push repeatedly?

You'd prefer Array$takeWhile to a generic findIndex? The definition of Array$takeWhile and Array$dropWhile will be nearly identical save the arguments to slice.

We don't have to expose it.

Plus we can just remove it once support for IE is dropped...in 2025 😜

I took the liberty of going the findIndex route. I can change it if you feel strongly.

davidchambers · 2017-03-06T16:11:24Z

index.js

      [Fn(a, $.Boolean), m(a), m(a)],
      Z.filterM);

+  //# takeWhile :: (Foldable f, Monoid (f a)) => (a -> Boolean) -> f a -> f a


I think we should replace Monoid (f a) with Alternative f for the reason given in sanctuary-js/sanctuary-type-classes#37.

btw here Alternative f constraint is used to express partiality
https://github.com/guaraqe/total-alternative#readme
https://github.com/guaraqe/total-alternative/blob/master/src/Data/List/Alternative.hs
https://github.com/guaraqe/total-alternative/blob/master/src/Data/Foldable/Alternative.hs

I think we updated the type signature without switching from Z.empty to Z.zero. I'll rectify this in the pull request I'll soon open for sanctuary-type-classes.

Actually, given our concat-based implementation I think Monoid (f a) is the correct constraint after all. We could have a chain-based takeWhileM (to match filterM) but it seems excessive to provide two versions of a function which is only used occasionally.

sanctuary-js/sanctuary-type-classes#69

davidchambers · 2017-03-06T22:46:27Z

index.js

+  function Array$findIndex(pred, xs) {
+    for (var idx = 0; idx < xs.length && pred(xs[idx]); idx += 1);
+    return idx;
+  }


I would expect Array$findIndex(x => x === 42, [1, 2, 3]) to evaluate to -1 rather than to 0. It seems this function isn't quite Array#findIndex after all.

My mistake. Even returning -1 it would still be incorrect. I was naming it findIndex but I can't really think of a good name for it...findFirstFailingIndexMinusOne.

I'll make Array$takeWhile and Array$dropWhile instead.

davidchambers · 2017-03-13T23:40:10Z

index.js

+  //. []
+  //. ```
+  function Array$takeWhile(pred, xs) {
+    for (var idx = 0; idx < xs.length && pred(xs[idx]); idx += 1);


My preference is to only declare block-scoped variables in loops (even though we don't actually have block scoping without let). Since we refer to idx outside the loop I'd like to declare it outside the loop:

var idx = 0; for (; idx < xs.length && pred(xs[idx]); idx += 1) {}

It would then make sense to switch to a while loop, as it would remove the awkward empty block:

var idx = 0; while (idx < xs.length && pred(xs[idx])) idx += 1;

The empty block is unnecessary. It can be replaced by a semicolon. But I'll make the change nonetheless.

davidchambers · 2017-03-13T23:42:17Z

I left one final comment, @gabejohnson. Please also rebase your branch. Let me know once you have done so (as I don't receive a notification when you --force push). I'll then merge this pull request. :)

gabejohnson · 2017-03-14T01:26:25Z

@davidchambers It should be good to merge now.

davidchambers reviewed Feb 27, 2017

View reviewed changes

svozza reviewed Feb 27, 2017

View reviewed changes

davidchambers reviewed Feb 27, 2017

View reviewed changes

gabejohnson force-pushed the add-takeWhile-dropWhile branch from ec3176a to 1a31f4f Compare February 27, 2017 14:49

davidchambers reviewed Mar 6, 2017

View reviewed changes

gabejohnson force-pushed the add-takeWhile-dropWhile branch from 1a31f4f to 19e8fb3 Compare March 6, 2017 19:06

davidchambers reviewed Mar 6, 2017

View reviewed changes

gabejohnson force-pushed the add-takeWhile-dropWhile branch from 19e8fb3 to 04df4f5 Compare March 7, 2017 04:04

davidchambers reviewed Mar 13, 2017

View reviewed changes

foldable: add S.takeWhile and S.dropWhile

951a7e7

gabejohnson force-pushed the add-takeWhile-dropWhile branch from 04df4f5 to 951a7e7 Compare March 14, 2017 01:19

davidchambers approved these changes Mar 14, 2017

View reviewed changes

davidchambers merged commit 05f2aa6 into sanctuary-js:master Mar 14, 2017

gabejohnson deleted the add-takeWhile-dropWhile branch September 19, 2017 14:44

Uh oh!

foldable: add S.takeWhile and S.dropWhile #356

foldable: add S.takeWhile and S.dropWhile #356

Uh oh!

Conversation

gabejohnson commented Feb 27, 2017

Uh oh!

gabejohnson commented Feb 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svozza Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabejohnson commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabejohnson Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabejohnson Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safareli Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

svozza Feb 27, 2017 •

edited

Loading

gabejohnson Mar 6, 2017 •

edited

Loading

gabejohnson Mar 6, 2017 •

edited

Loading

safareli Mar 6, 2017 •

edited

Loading

gabejohnson Mar 7, 2017 •

edited

Loading