Add S.fromPairs
#454
Add S.fromPairs
#454
Conversation
|
Looks very good! I'll leave a few comments, but there's not much to fault here. :) |
index.js
Outdated
| //. | ||
| //. ```javascript | ||
| //. > S.fromPairs([['a', 1], ['b', 2], ['c', 3]]) | ||
| //. {b: 2, a: 1, c: 3} |
There was a problem hiding this comment.
Is there a reason to write {b: 2, a: 1, c: 3} rather than {a: 1, b: 2, c: 3}?
There was a problem hiding this comment.
Copy-pasted from S.pairs. I guess it's a hint on the arbitrary order of things? Should we: leave it as is, sort only fromPairs, or sort both pairs and fromPairs examples?
There was a problem hiding this comment.
Oh, wait, I see that S.pairs example is sorted explicitly with .sort(). I'll just sort the keys in fromPairs example then.
index.js
Outdated
| //. {b: 2, a: 1, c: 3} | ||
| //. ``` | ||
| function fromPairs(pairs) { | ||
| return Z.reduce(function(m, pair) { |
There was a problem hiding this comment.
I'd prefer to name the first parameter strMap.
index.js
Outdated
| @@ -3778,6 +3778,24 @@ | |||
| S.pairs = | |||
| def('pairs', {}, [$.StrMap(a), $.Array($.Pair($.String, a))], pairs); | |||
|
|
|||
| //# fromPairs :: Array (Pair String a) -> StrMap a | |||
| //. | |||
| //. Returns a new string map formed by a list of key-value pairs. | |||
There was a problem hiding this comment.
I think this is a slight improvement:
Returns a string map containing the key–value pairs specified by the given array.
Note: Please use an en dash in key–value pairs. :)
test/fromPairs.js
Outdated
| eq(S.fromPairs([['a', 1], ['b', 2], ['c', 3]]), {a: 1, b: 2, c: 3}); | ||
|
|
||
| eq(S.fromPairs([['x', 1], ['x', 2]]), {x: 2}); | ||
| }); |
There was a problem hiding this comment.
Nit: Please delete line 16 and add an empty line between lines 17 and 18.
|
This looks great to me! Please squash the commits. I'll leave this pull request open for a couple of days to give other Sanctuary contributors an opportunity to comment. |
test/fromPairs.js
Outdated
|
|
||
| eq(S.fromPairs([]), {}); | ||
| eq(S.fromPairs([['a', 1], ['b', 2], ['c', 3]]), {a: 1, b: 2, c: 3}); | ||
| eq(S.fromPairs([['x', 1], ['x', 2]]), {x: 2}); |
There was a problem hiding this comment.
I just realized that this would also make an excellent doctest. It neatly demonstrates the second sentence of the description. Would you mind adding this to the ```javascript block as well?
index.js
Outdated
| }, {}, pairs); | ||
| } | ||
| S.fromPairs = | ||
| def('fromPairs', {}, [$.Array($.Pair($.String, a)), $.StrMap(a)], fromPairs); |
There was a problem hiding this comment.
Since the function uses Z.reduce, maybe this signature could be expanded to:
Foldable f => f (Pair String a) -> StrMap aThere was a problem hiding this comment.
Good idea! Let's replace array with [Foldable][] in the description while we're at it. :)
index.js
Outdated
| def('fromPairs', | ||
| {f: [Z.Foldable]}, | ||
| [f($.Pair($.String, a)), $.StrMap(a)], | ||
| fromPairs); |
There was a problem hiding this comment.
Nit: Please indent the three lines above so that all four arguments to def are aligned. :)
index.js
Outdated
| //# fromPairs :: Foldable f => f (Pair String a) -> StrMap a | ||
| //. | ||
| //. Returns a string map containing the key–value pairs specified by | ||
| //. the given [Foldable][]. If a key appears in multiple pairs, the rightmost |
There was a problem hiding this comment.
Nit: Let's make the lengths of the two lines above more even by moving the first word of the second line to the end of the first line. :)
|
The changes look good, @futpib! I'll merge this pull request as soon as you've made the two minor formatting changes. |
|
I really suspect you are kidding me at this point. |
|
Thank you for your patience, @futpib. 😄 |
No description provided.