add apFirst and apSecond #11

zhangchiqing · 2016-11-05T13:53:42Z

I just moved the changes from sanctuary-js/sanctuary#270 (comment) to here.

I didn't change the function names, as they are still in discussion.

Please review.

I will create another PR to sanctuary-js/sanctuary#216 once this PR has been merged.

davidchambers · 2016-11-05T16:25:58Z

README.md

@@ -50,7 +50,7 @@ This project provides:

 ## API

-<h4 name="TypeClass"><code><a href="https://github.com/sanctuary-js/sanctuary-type-classes/blob/v1.0.0/index.js#L105">TypeClass :: (String, Array TypeClass, a -> Boolean) -> TypeClass</a></code></h4>
+<h4 name="TypeClass"><code><a href="https://github.com/sanctuary-js/sanctuary-type-classes/blob/v1.0.0/index.js#L112">TypeClass :: (String, Array TypeClass, a -> Boolean) -> TypeClass</a></code></h4>


As mentioned in the CONTRIBUTING.md, it's not necessary to update README.md manually. ;)

davidchambers · 2016-11-05T16:27:32Z

index.js

@@ -88,6 +88,13 @@
  //  identity :: a -> a
  var identity = function(x) { return x; };

+  // kCombinator :: a -> b -> a
+  var kCombinator = function(x) {


Let's name this constant for consistency with identity.

davidchambers · 2016-11-05T16:28:53Z

index.js

@@ -1056,6 +1063,42 @@
    return ap(ap(map(f, x), y), z);
  };

+  //# apFirst :: Apply f => f a -> f b -> f a


Since this function is not curried, the signature should be Apply f => (f a, f b) -> f a.

davidchambers · 2016-11-05T16:30:37Z

index.js

+  //# apFirst :: Apply f => f a -> f b -> f a
+  //.
+  //. Combine two effectful actions, keeping only the result of the first.
+  //. Equivalent to Haskell's <* function.


Let's write (<*) rather than <*.

davidchambers · 2016-11-05T16:31:18Z

index.js

+  //. > apFirst([1, 2], [3, 4])
+  //. [1, 1, 2, 2]
+  //.
+  //. ```javascript


This needs to move up a few lines.

zhangchiqing · 2016-11-06T01:52:04Z

Thanks for the review @davidchambers .

I've updated the PR for all your comments. And reverted the changes to README.md.

Please let me know they are good :)

davidchambers · 2016-11-06T01:56:52Z

index.js

@@ -88,6 +88,13 @@
  //  identity :: a -> a
  var identity = function(x) { return x; };

+  // constant :: a -> b -> a
+  var constant = function(x) {


Please add one more space between // and constant to align the function name in type line with the identifier on the following line. :)

Also, for lack of a better system the helper functions are defined in alphabetical order, so constant should be defined before identity.

davidchambers · 2016-11-06T01:58:36Z

index.js

+  //# apFirst :: Apply f => (f a, f b) -> f a
+  //.
+  //. Combine two effectful actions, keeping only the result of the first.
+  //. Equivalent to Haskell's (<*) function.


Let's wrap the function name in backticks so it will be rendered as "(<*)" rather than "(<*)".

zhangchiqing · 2016-11-06T02:45:40Z

Good advices!

Just updated, please take a look again, thanks @davidchambers

davidchambers · 2016-11-06T03:21:22Z

Thanks very much for submitting this pull request, @zhangchiqing, and for updating it quickly in response to my suggestions.

I'd like to leave this open for a day or two while we decide on the most appropriate names, but otherwise this looks ready to merge. :)

zhangchiqing · 2016-11-06T03:35:41Z

Good to know :)

Yes, I agree. Please let me know if we need to rename them.

Thanks again for the code review.

safareli · 2016-11-06T09:42:46Z

@scott-christopher I agree left/right could be confusing because of Either type, in that case I think first/last is better option.

davidchambers · 2016-11-06T19:22:53Z

Are you happy with this pull request as it stands, @safareli?

safareli · 2016-11-06T19:46:12Z

I would like if we use apLast instead of apSecond, but if not it's not that big issue.

davidchambers · 2016-11-06T22:35:53Z

They're essentially binary operators, aren't they? I think of the blanks in _ *> _ as left-hand operand and right-hand operand, or as first operand and second operand. The word last suggests to me we're dealing with a variable number of things rather than exactly two things.

safareli · 2016-11-07T09:46:56Z

Agree!

davidchambers · 2016-11-07T09:50:36Z

index.js

@@ -1272,6 +1315,8 @@
    ap: ap,
    lift2: lift2,
    lift3: lift3,
+    apFirst: apFirst,
+    apSecond: apSecond,


One last thing: I think apFirst and apSecond should come between ap and lift. Could you move the definitions, the exports, and the tests?

Do you mean var apFirst = ... should be placed between var ap = ... and var lift2 = ...?

Can I ask what's the idea behind it :)

The thing is I implemented apFirst and apSecond using lift2. If I move the definition above lift2, it looks like apFirst is using a function (lift2) that is not defined yet.

Or I can place apFirst between ap and lift2 and refactor apFirst in terms of ap and map as below, since ap and map are both defined above apFirst and apSecond.

var apFirst = function apFirst(x, y) { return ap(map(constant, x), y); };

I think you can still have access to lift2 if you move apFirst up, because of js hoisting

I think you can still have access to lift2 if you move apFirst up

Yeah, I agree. Syntax-wise it would work.

Do you think one day 'lift2` will support currying? In which case:

var apFirst = lift2(constant);

Won't work, because lift2 is not defined yet.

Also I'm going to add apFirst and apSecond to sanctuary.js after this PR has been merged. Since lift2 is curried in sanctuary.js, I can implement them like this var apFirst = S.lift2(S.I);, but I can't place them above S.lift2.

Good point, @zhangchiqing. The location you chose is perfect! I'll merge this when I get home. :)

davidchambers · 2016-11-07T20:19:26Z

Thanks again for this nice contribution, @zhangchiqing. Don't worry about creating a Sanctuary pull request; I will update sanctuary-js/sanctuary#216 to include S.apFirst and S.apSecond.

🌳

zhangchiqing · 2016-11-07T20:57:07Z

Awesome! I'm more than happy to contribute :)

zhangchiqing · 2016-11-07T21:03:39Z

One more question:

Will the README.md be updated with the new functions once this PR gets merged?

Should we run make all manually?

safareli · 2016-11-08T09:43:26Z

readme is updated when version is bumped

davidchambers · 2016-11-08T11:02:15Z

@zhangchiqing, see sanctuary-js/sanctuary-set#1 (comment) if you'd like to know how and when the readme is updated. :)

davidchambers reviewed Nov 5, 2016

View reviewed changes

index.js

//. > apFirst([1, 2], [3, 4])

//. [1, 1, 2, 2]

//.

//. ```javascript

Copy link

Member

davidchambers Nov 5, 2016

Choose a reason for hiding this comment

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

This needs to move up a few lines.

zhangchiqing force-pushed the feature-add-apFirst-apSecond branch from 62f1e98 to 787980b Compare November 6, 2016 01:45

davidchambers reviewed Nov 6, 2016

View reviewed changes

add apFirst and apSecond

17eef50

zhangchiqing force-pushed the feature-add-apFirst-apSecond branch from 787980b to 17eef50 Compare November 6, 2016 02:44

davidchambers mentioned this pull request Nov 6, 2016

apply: add apFirst, apSecond sanctuary-js/sanctuary#270

Closed

davidchambers reviewed Nov 7, 2016

View reviewed changes

davidchambers merged commit 6fb76ad into sanctuary-js:master Nov 7, 2016

zhangchiqing deleted the feature-add-apFirst-apSecond branch November 7, 2016 20:57

zhangchiqing mentioned this pull request Nov 8, 2016

S.apFirst and S.apSecond sanctuary-js/sanctuary#269

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add apFirst and apSecond #11

add apFirst and apSecond #11

zhangchiqing commented Nov 5, 2016

davidchambers Nov 5, 2016

davidchambers Nov 5, 2016

davidchambers Nov 5, 2016

davidchambers Nov 5, 2016

davidchambers Nov 5, 2016

zhangchiqing commented Nov 6, 2016

davidchambers Nov 6, 2016

davidchambers Nov 6, 2016

zhangchiqing commented Nov 6, 2016

davidchambers commented Nov 6, 2016

zhangchiqing commented Nov 6, 2016

safareli commented Nov 6, 2016

davidchambers commented Nov 6, 2016

safareli commented Nov 6, 2016

davidchambers commented Nov 6, 2016

safareli commented Nov 7, 2016

davidchambers Nov 7, 2016

zhangchiqing Nov 7, 2016

safareli Nov 7, 2016 •

edited

Loading

zhangchiqing Nov 7, 2016 •

edited

Loading

davidchambers Nov 7, 2016

davidchambers commented Nov 7, 2016

zhangchiqing commented Nov 7, 2016

zhangchiqing commented Nov 7, 2016

safareli commented Nov 8, 2016

davidchambers commented Nov 8, 2016

add apFirst and apSecond #11

add apFirst and apSecond #11

Conversation

zhangchiqing commented Nov 5, 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

zhangchiqing commented Nov 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangchiqing commented Nov 6, 2016

davidchambers commented Nov 6, 2016

zhangchiqing commented Nov 6, 2016

safareli commented Nov 6, 2016

davidchambers commented Nov 6, 2016

safareli commented Nov 6, 2016

davidchambers commented Nov 6, 2016

safareli commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safareli Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

zhangchiqing Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchambers commented Nov 7, 2016

zhangchiqing commented Nov 7, 2016

zhangchiqing commented Nov 7, 2016

safareli commented Nov 8, 2016

davidchambers commented Nov 8, 2016

safareli Nov 7, 2016 •

edited

Loading

zhangchiqing Nov 7, 2016 •

edited

Loading