Add S.append #184

gilligan · 2016-04-19T21:47:33Z

This adds S.append as suggested by #145

So this is my first contribution and I just naively implemented it as a -> [a] -> [a].

svozza · 2016-04-19T21:51:56Z

We could restrict a to Semigroup, perhaps?

davidchambers · 2016-04-19T21:54:05Z

Thanks, @gilligan!

Let's move the code and tests to the List section. It'd feel at home between dropLast and find, I'd say.

kedashoe · 2016-04-19T21:56:33Z

index.js

+  def('append',
+      {},
+      [a, $.Array(a), $.Array(a)],
+      function(x, xs) { return xs.concat(x); });


I think we need to wrap x here, currently we have S.append([3, 4], [[1], [2]]); //=> [[1], [2], 3, 4]

Let's use xs.concat([x]), to handle nested arrays correctly:

> [1, 2].concat(3) [1, 2, 3] > [1, 2].concat([3]) [1, 2, 3] > [[1], [2]].concat([3]) [[1], [2], 3] > [[1], [2]].concat([[3]]) [[1], [2], [3]]

Let's also include a test case to cover this.

Beaten to the punch! Good spotting, Kevin. :)

gilligan · 2016-04-19T22:02:39Z

@svozza I guess semigroup could make sense yes.
@davidchambers sure, will update the PR tomorrow.

davidchambers · 2016-04-19T22:05:38Z

We could restrict a to Semigroup, perhaps?

I don't think so, Stefano. The function should work forall a. The container must support concat, of course, but that's ensured by our use of $.Array.

gilligan · 2016-04-19T22:07:12Z

@davidchambers right.. so S.append(() => {}, []) should of course work as well.

svozza · 2016-04-19T22:23:13Z

I don't think so, Stefano. The function should work forall a. The container must support concat, of course, but that's ensured by our use of $.Array.

Do we not want this to work on strings too?

davidchambers · 2016-04-19T23:14:41Z

Do we not want this to work on strings too?

I'm not sure this is necessary: S.concat('foo', '!') behaves as S.append('!', 'foo') presumably would. One thing that worries me about supporting strings here is that S.append('!') would do different things depending on the type of the second argument:

> S.append('!', 'abc')
'abc!'

> S.append('!', ['a', 'b', 'c'])
['a', 'b', 'c', '!']

There's also the question of whether S.append('bar', 'foo') is a type error, since it'd append more than one character to 'foo'. Deferring to S.concat is a reasonable solution to these conundrums.

gilligan · 2016-04-20T17:48:46Z

@davidchambers

changed to concat([x])
added unit test / doc test to append list to list of lists
moved implementation list section
moved test to list section

anything else?

davidchambers · 2016-04-20T18:19:29Z

index.js

@@ -3032,6 +3065,8 @@
      [$.Array($.String), $.String],
      compose(R.join(''), R.map(concat(_, '\n'))));

+
+


davidchambers · 2016-04-26T00:55:48Z

index.js

+  //# append :: a -> [a] -> [a]
+  //.
+  //. Takes a value of any type and a list of values of that type, and
+  //. returns the result of appending the value to the list.


In light of #180, we need to make a few changes to preserve consistency:

change the signature to a -> Array a -> Array a;

s/a list/an array/;

s/the list/the array/; and

move the function to the Array section (at the very beginning, let's say).

Much has changed in the few days since you submitted this pull request. I realize I've already asked you to move this function once! There won't usually be this much friction, I promise. :)

davidchambers · 2016-04-26T00:56:33Z

It looks as though your tests disappeared in the rebase!

gilligan · 2016-04-26T09:18:22Z

@davidchambers ugh.. tests added again. This whole PR took much longer than I would have anticipated ;)

Avaq · 2016-04-26T10:00:59Z

index.js

@@ -2274,6 +2274,7 @@
        return n < 0 || negativeZero(n) ? Nothing() : slice(0, -n, xs);
      });

+


Is this newline intentional?

Avaq · 2016-04-26T10:43:02Z

Looks good to me, though I haven't kept up with the discussions leading up to this point. I'll leave the final call to @davidchambers.

davidchambers · 2016-04-26T15:07:27Z

index.js

@@ -2292,6 +2292,24 @@
               R.pipe(R[name], Just, R.filter(R.gte(_, 0))));
  };

+  //# append :: a -> Array a -> Array a


This is still in the List section. This function is Array-specific so belongs in the Array section. Let's do this:

//. ### Array + //# append :: a -> Array a -> Array a + //. + //. Takes a value of any type and an array of values of that type, and + //. returns the result of appending the value to the array. + //. + //. ```javascript + //. > S.append(3, [1, 2]) + //. [1, 2, 3] + //. + //. > S.append([3, 4], [[1], [2]]) + //. [[1], [2], [3, 4]] + //. ``` + S.append = + def('append', + {}, + [a, $.Array(a), $.Array(a)], + function(x, xs) { return xs.concat([x]); }); + //# find :: (a -> Boolean) -> Array a -> Maybe a

wow i'm close to giving up x__X - The one huge file is driving me bonkers. I don't think I've ever needed so many attempts to get something so trivial done. Sorry.

Even with separate files we'd need to specify the readme order somewhere, but a list of filenames in the makefile would certainly be easier to read. ;)

gilligan · 2016-04-27T16:30:59Z

Are we good to go now? :-) fingers crossed

davidchambers · 2016-04-27T16:47:16Z

index.js

@@ -2292,6 +2292,7 @@
               R.pipe(R[name], Just, R.filter(R.gte(_, 0))));
  };

+


davidchambers · 2016-04-27T16:48:17Z

Delete the stray line and I'll merge. :)

davidchambers · 2016-04-28T04:38:24Z

🌳 Thanks, Tobias!

kedashoe reviewed Apr 19, 2016
View reviewed changes

gilligan force-pushed the add-append-function branch 3 times, most recently from 0504a2d to c6ed504 Compare April 20, 2016 17:48

davidchambers reviewed Apr 20, 2016
View reviewed changes

index.js

@@ -3032,6 +3065,8 @@

[$.Array($.String), $.String],

compose(R.join(''), R.map(concat(_, '\n'))));

Copy link

Member

davidchambers Apr 20, 2016

Choose a reason for hiding this comment

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

👾

gilligan force-pushed the add-append-function branch 7 times, most recently from 6da9370 to 3002ecb Compare April 25, 2016 19:06

davidchambers reviewed Apr 26, 2016
View reviewed changes

gilligan force-pushed the add-append-function branch 2 times, most recently from ebdfd02 to 2804380 Compare April 26, 2016 08:08

Avaq reviewed Apr 26, 2016
View reviewed changes

index.js

@@ -2274,6 +2274,7 @@

return n < 0 || negativeZero(n) ? Nothing() : slice(0, -n, xs);

});

Copy link

Member

Avaq Apr 26, 2016

Choose a reason for hiding this comment

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

Is this newline intentional?

gilligan force-pushed the add-append-function branch from 2804380 to f913cc1 Compare April 26, 2016 10:34

davidchambers reviewed Apr 26, 2016
View reviewed changes

gilligan force-pushed the add-append-function branch from f913cc1 to 3c7aa32 Compare April 26, 2016 15:41

davidchambers reviewed Apr 27, 2016
View reviewed changes

index.js

@@ -2292,6 +2292,7 @@

R.pipe(R[name], Just, R.filter(R.gte(_, 0))));

};

Copy link

Member

davidchambers Apr 27, 2016

Choose a reason for hiding this comment

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

👾

Add S.append

f36353f

gilligan force-pushed the add-append-function branch from 3c7aa32 to f36353f Compare April 27, 2016 18:40

davidchambers merged commit 281f647 into sanctuary-js:master Apr 28, 2016

gilligan deleted the add-append-function branch April 28, 2016 06:13

davidchambers mentioned this pull request May 31, 2016

v0.11.1 release notes #227

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add S.append #184

Add S.append #184

gilligan commented Apr 19, 2016

svozza commented Apr 19, 2016

davidchambers commented Apr 19, 2016

kedashoe Apr 19, 2016

davidchambers Apr 19, 2016

davidchambers Apr 19, 2016

gilligan commented Apr 19, 2016

davidchambers commented Apr 19, 2016 •

edited by svozza

Loading

gilligan commented Apr 19, 2016

svozza commented Apr 19, 2016

davidchambers commented Apr 19, 2016

gilligan commented Apr 20, 2016

davidchambers Apr 20, 2016

davidchambers Apr 26, 2016

davidchambers commented Apr 26, 2016

gilligan commented Apr 26, 2016

Avaq Apr 26, 2016

Avaq commented Apr 26, 2016

davidchambers Apr 26, 2016

gilligan Apr 26, 2016

davidchambers Apr 26, 2016

gilligan commented Apr 27, 2016

davidchambers Apr 27, 2016

davidchambers commented Apr 27, 2016

davidchambers commented Apr 28, 2016

		@@ -3032,6 +3065,8 @@
		[$.Array($.String), $.String],
		compose(R.join(''), R.map(concat(_, '\n'))));

		@@ -2274,6 +2274,7 @@
		return n < 0 \|\| negativeZero(n) ? Nothing() : slice(0, -n, xs);
		});

		@@ -2292,6 +2292,7 @@
		R.pipe(R[name], Just, R.filter(R.gte(_, 0))));
		};

Add S.append #184

Add S.append #184

Conversation

gilligan commented Apr 19, 2016

svozza commented Apr 19, 2016

davidchambers commented Apr 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilligan commented Apr 19, 2016

davidchambers commented Apr 19, 2016 • edited by svozza Loading

gilligan commented Apr 19, 2016

svozza commented Apr 19, 2016

davidchambers commented Apr 19, 2016

gilligan commented Apr 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchambers commented Apr 26, 2016

gilligan commented Apr 26, 2016

Choose a reason for hiding this comment

Avaq commented Apr 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilligan commented Apr 27, 2016

Choose a reason for hiding this comment

davidchambers commented Apr 27, 2016

davidchambers commented Apr 28, 2016

davidchambers commented Apr 19, 2016 •

edited by svozza

Loading