Added function S.on #282

Bradcomp · 2016-11-14T01:41:33Z

Closes #281

davidchambers · 2016-11-14T10:12:06Z

Thanks for the pull request, @Bradcomp! There are a few minor things on which I will comment, but overall it looks very good. :)

davidchambers · 2016-11-14T10:21:53Z

index.js

@@ -713,6 +713,25 @@
      [$.Function, a, b, c, d],
      lift3);

+  //# on :: (b -> b -> c) -> (a -> b) -> a -> a -> c


We must decide whether the type of the first argument should be b -> b -> c or (b, b) -> c. Currently the type signature and the implementation contradict each other.

There are a handful of higher-order functions for which we provide both forms. In each case S.<name> takes a curried function while S.<name>_ takes an uncurried function. We could provide both S.on and S.on_, but S.on should take a b -> b -> c function regardless.

davidchambers · 2016-11-14T10:29:18Z

test/on.js

+
+  it('should pass the last two arguments through the modifying function into the converging function', function() {
+    eq(S.on(S.max, S.prop('x'), {x: 3, y: 5}, {x: 5, y: 3}), 5);
+    eq(S.on(S.concat, S.reverse)([1, 2, 3], [4, 5, 6]), [3, 2, 1, 6, 5, 4]);


Because Sanctuary functions can be applied to their arguments one at a time or all at once, they're not well suited to testing higher-order functions. We should use rem or rem_ to differentiate between f(x)(y) and f(x, y) in the implementation.

davidchambers · 2016-11-14T10:31:41Z

index.js

+  def('on',
+  {},
+  [$.Function, $.Function, a, a, c],
+  on);


It would be fine to have this on one line:

S.on = def('on', {}, [$.Function, $.Function, a, a, c], on);

davidchambers · 2016-11-14T10:47:39Z

index.js

+  //. results through the binary function.
+  //.
+  //. ```javascript
+  //. S.on(S.concat, S.reverse, [1, 2, 3], [4, 5, 6])


This line is missing the > which indicates a doctest input line.

davidchambers · 2016-11-14T10:57:54Z

index.js

+  {},
+  [$.Function, $.Function, a, a, c],
+  on);
+
  //. ### Composition


Perhaps on should live in the Composition section rather than the Function section. When I consider what it does I imagine something like this:

x y | | | | | | | | | | +-----+ +-----+ | g | | g | +-----+ +-----+ \ / \ / g(x) \ / g(y) \ / \ / +-----+ | f | +-----+ | | | | | f(g(x))(g(y))

davidchambers · 2016-11-14T10:58:49Z

test/on.js

+
+var eq = require('./internal/eq');
+
+describe('on', function() {


Nit: Two empty lines after imports, please.

davidchambers · 2016-11-14T11:00:12Z

test/on.js

+
+describe('on', function() {
+
+  it('has an arity of four', function() {


For consistency, let's write 'is a quaternary function'.

davidchambers · 2016-11-15T10:57:28Z

index.js

+  //.
+  //. Takes a binary function, a unary function, and two values. Returns the
+  //. result of applying the unary function to each value, then passing the
+  //. results through the binary function.


"Passing through" doesn't sound right to me for some reason. Combinators are sometimes best explained by their definitions. We could write something along these lines:

Takes a binary function f, a unary function g, and two values x and y. Returns f(g(x))(g(y)).

davidchambers · 2016-11-15T10:57:59Z

test/on_.js

+var rem_ = require('./internal/rem_');
+
+
+describe('on', function() {


s/on/on_/

davidchambers · 2016-11-15T10:59:15Z

test/on_.js

+
+
+describe('on', function() {
+


Let's include an 'is a quaternary function' block for this function too.

davidchambers · 2016-11-15T11:01:52Z

test/on.js

+    eq(S.on.length, 4);
+  });
+
+  it('should pass the last two arguments through the modifying function into the converging function', function() {


Nit: I prefer it does X to it should do X. In this case, it passes the last two arguments…

davidchambers · 2016-11-15T11:06:16Z

Thanks for the updates. Providing both on and on_ will be convenient.

One downside of my preference for rebasing is that I don't receive notifications when pull requests are updated. Please leave a comment after making the second round of changes to let me know I should take one last look. :)

Bradcomp · 2016-11-15T15:22:35Z

Thanks for your feedback! I've updated the PR based on your comments.

davidchambers · 2016-11-15T18:17:27Z

index.js

+  //# on :: (b -> b -> c) -> (a -> b) -> a -> a -> c
+  //.
+  //. Takes a binary function `f`, a unary function `g`, and two
+  //. values `x` and `y`. Returns f(g(x))(g(y)).


Let's wrap f(g(x))(g(y)) in backticks.

davidchambers · 2016-11-15T18:19:25Z

I'll merge this once you've made the final copy tweak. I appreciate your patience. :)

Bradcomp · 2016-11-16T01:51:36Z

Alright! I think we should be good to go. Thanks for you time in looking this over and considering it!

davidchambers · 2016-11-16T02:00:57Z

🌳

davidchambers · 2017-02-13T00:55:17Z

$ npm install [email protected]

$ node
> const S = require('sanctuary')
undefined
> S.on
on :: (b -> b -> c) -> (a -> b) -> a -> a -> c
> S.on_
on_ :: ((b, b) -> c) -> (a -> b) -> a -> a -> c

davidchambers reviewed Nov 14, 2016

View reviewed changes

test/on.js

var eq = require('./internal/eq');

describe('on', function() {

Copy link

Member

davidchambers Nov 14, 2016

Choose a reason for hiding this comment

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

Nit: Two empty lines after imports, please.

davidchambers reviewed Nov 14, 2016

View reviewed changes

Bradcomp force-pushed the on branch from 7a20c1f to 812af55 Compare November 15, 2016 02:41

davidchambers reviewed Nov 15, 2016

View reviewed changes

test/on_.js

var rem_ = require('./internal/rem_');

describe('on', function() {

Copy link

Member

davidchambers Nov 15, 2016

Choose a reason for hiding this comment

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

s/on/on_/

davidchambers reviewed Nov 15, 2016

View reviewed changes

test/on_.js

describe('on', function() {

Copy link

Member

davidchambers Nov 15, 2016

Choose a reason for hiding this comment

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

Let's include an 'is a quaternary function' block for this function too.

davidchambers reviewed Nov 15, 2016

View reviewed changes

Bradcomp force-pushed the on branch from 812af55 to a98934e Compare November 15, 2016 15:19

davidchambers reviewed Nov 15, 2016

View reviewed changes

Added functions S.on and S.on_

130c293

Bradcomp force-pushed the on branch from a98934e to 130c293 Compare November 16, 2016 01:32

davidchambers merged commit 05e0150 into sanctuary-js:master Nov 16, 2016

davidchambers mentioned this pull request Feb 13, 2017

v0.12.0 release notes #332

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added function S.on #282

Added function S.on #282

Bradcomp commented Nov 14, 2016

davidchambers commented Nov 14, 2016

davidchambers Nov 14, 2016

davidchambers Nov 14, 2016

davidchambers Nov 14, 2016

davidchambers Nov 14, 2016

davidchambers Nov 14, 2016

davidchambers Nov 14, 2016

davidchambers Nov 14, 2016

davidchambers Nov 15, 2016

davidchambers Nov 15, 2016

davidchambers Nov 15, 2016

davidchambers Nov 15, 2016

davidchambers commented Nov 15, 2016

Bradcomp commented Nov 15, 2016

davidchambers Nov 15, 2016

davidchambers commented Nov 15, 2016

Bradcomp commented Nov 16, 2016

davidchambers commented Nov 16, 2016

davidchambers commented Feb 13, 2017


		var eq = require('./internal/eq');

		describe('on', function() {


		describe('on', function() {

		it('has an arity of four', function() {

		var rem_ = require('./internal/rem_');


		describe('on', function() {

Added function S.on #282

Added function S.on #282

Conversation

Bradcomp commented Nov 14, 2016

davidchambers commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchambers commented Nov 15, 2016

Bradcomp commented Nov 15, 2016

Choose a reason for hiding this comment

davidchambers commented Nov 15, 2016

Bradcomp commented Nov 16, 2016

davidchambers commented Nov 16, 2016

davidchambers commented Feb 13, 2017