Add S.keys, S.values and S.pairs #214

svozza · 2016-05-08T22:11:56Z

Creating a type-safe version of R.keys was mentioned in #142 and now that StrMap is available I thought I'd road test it a bit. The implementation of this will probably provoke discussion, firstly because I'm using an ES5 only implementation: I find the Ramda implementation far too complicated for what should be a simple function. Secondly, I've chosen not to implement S.values in terms of S.keys because I don't think it makes sense to pay the penalty of type checking the object twice.

davidchambers · 2016-05-09T00:04:00Z

I'm excited to see these functions; they'll make great additions to the library!

I'd also like to provide pairs :: StrMap a -> Array (Pair String a). I'd like pairs, keys, and values to guarantee consistent ordering. This would mean one could apply keys and values to a value independently and later zip the results, confident that the pairs will align correctly.

We could rely on the default behaviour of Array#sort.

The implementation of this will probably provoke discussion, firstly because I'm using an ES5 only implementation: I find the Ramda implementation far too complicated for what should be a simple function.

The Ramda implementation is complicated because it works around bugs in certain old browser versions. We needn't do this. The ES3-compatible implementations needn't be complex:

//  keys :: StrMap a -> Array String
var keys = function(strMap) {
  var ks = [];
  for (var k in strMap) if (has(k, strMap)) ks.push(k);
  return ks.sort();
}

//  values :: StrMap a -> Array a
var values = function(strMap) {
  var ks = keys(strMap);
  var vs = [];
  for (var idx = 0; idx < ks.length; idx += 1) {
    vs.push(strMap[ks[idx]]);
  }
  return vs;
};

//  pairs :: StrMap a -> Array (Pair String a)
var pairs = function(strMap) {
  var ks = keys(strMap);
  var kvs = [];
  for (var idx = 0; idx < ks.length; idx += 1) {
    kvs.push([ks[idx], strMap[ks[idx]]]);
  }
  return kvs;
}

I don't think it makes sense to pay the penalty of type checking the object twice.

Type checking is currently very slow. I'm confident we can reduce the cost by at least 90%. Once one of us spends a few days making the type checking less naive we'll hopefully be able to stop worrying about paying the cost twice.

kedashoe · 2016-05-09T04:56:51Z

I'd like pairs, keys, and values to guarantee consistent ordering.

What about using @svozza's implementations for these names, and also adding sortedKeys, sortedValues, sortedPairs for when consistent ordering is needed? Iterating over a collection 3-ish times to get values feels like a bit much to me.

And what about a simple for (key in obj) to get the values in values?

davidchambers · 2016-05-09T05:12:43Z

I'm open to providing "fast" versions of these functions as well, but I'd like to name them keys_, values_, and pairs_. I'd like the "nice" versions of the functions to have the nice names.

The other option is to sidestep the ordering problem by defining Set and Bag types. We could then define:

keys   :: StrMap a -> Set String
values :: StrMap a -> Bag a
pairs  :: StrMap a -> Set (Pair String a)

I'd like to stick with arrays, though, so we could guarantee this property:

forall m :: StrMap a. R.zip(S.keys(m), S.values(m)) = S.pairs(m)

kedashoe · 2016-05-09T05:23:01Z

For me the faster versions make sense with the plain names because

I almost never care about the order of these functions
It seems more natural to me to think "do I care about order here" than "do I care about performance here"

But I see the appeal of ordered results to these functions.. I'm okay either way 🤷

davidchambers · 2016-05-09T05:38:24Z

I might be on the wrong side of the fence here. :)

What do others think? @Avaq? @benperez? @scott-christopher? I'm also interested in your thoughts on the matter, @CrossEye, since we've discussed this in Ramda threads several times.

svozza · 2016-05-09T15:27:58Z

I've ES3-ified the implementation. Do you want me to add pairs to this PR?

Regarding sorting the keys, it seems odd to impose an order of keys in a map, if the user really cares she should just sort the result herself.

davidchambers · 2016-05-09T19:04:26Z

I've ES3-ified the implementation.

Thanks. We're still relying on Array#map in values, though.

Do you want me to add pairs to this PR?

If you'd like to do so, sure! It's up to you. :)

Regarding sorting the keys, it seems odd to impose an order of keys in a map, if the user really cares she should just sort the result herself.

Sorting the array returned by keys is not so important: as you rightly point out the caller can do this after the fact. She cannot, though, sort the values by their keys after the fact, as the keys are gone.

The question is whether to support diverging/converging pipelines such as this:

                          {x: 0, y: 42}
                               / \
                              /   \
                             /     \
                            /       \
                           /         \
                          /           \
         ['x', 'y'] <=  keys        values  => [0, 42]
                         |             |
                         |             |
                         |             |
                         |             |
                         |             |
                         |             |
     ['X', 'Y'] <=  map(toUpper)   map(inc)  => [1, 43]
                          \           /
                           \         /
                            \       /
                             \     /
                              \   /
                               \ /
                               zip  => [['X', 1], ['Y', 43]]
                                |
                                |
                                |
                                |
                                |
                                |
                            fromPairs  => {X: 1, Y: 43}

If we decide not to guarantee consistent ordering, the user can of course use this approach instead:

                          {x: 0, y: 42}
                                |
                                |
                                |
                                |
                              pairs  => [['x', 0], ['y', 42]]
                                |
                                |
                                |
                                |
                 map(over(lensIndex(0), toUpper))  => [['X', 0], ['Y', 42]]
                                |
                                |
                                |
                                |
                   map(over(lensIndex(1), inc))  => [['X', 1], ['Y', 43]]
                                |
                                |
                                |
                                |
                            fromPairs  => {X: 1, Y: 43}

Both approaches seem fine to me. If we support both, users are free to use whichever feels more natural. Forcing users to think about such flows in a certain way doesn't appeal to me (though I realize Sanctuary is an opinionated library which forces users to think a certain way in many other contexts).

Does this clarify my position, @svozza? As I say, I might be on the wrong side of this disagreement. ;)

davidchambers · 2016-05-09T19:06:45Z

index.js

@@ -362,6 +364,14 @@
        }
      });

+  //  keys :: StrMap a -> Array String
+  var keys = function(strMap) {
+    if (typeof Object.keys === 'function') return Object.keys(strMap);


Is this for performance? My default position is to minimize branching, but I'm happy to introduce branching if there's a significant performance difference.

I remember reading before that Object.keys is heavily optimised but I can't for the life of me remember where. for...in loops are supposed to be slow too, especially the hasOwnProperty guard. Although tbh, I'm not sure any of that matters, I can't imagine people using this function on objects with enough keys for it to be an issue.

I would advice against optimizing performance before

It's established that there is a performance issue

It's established that the optimization fixes it

The optimized code behaves exactly the same as the non-optimized code

That said, a quick performance benchmark on my local machine shows that Object.keys is about 5 times as fast as for ... in hasOwn, so that certainly crosses out 2 from my list. ;)

I'm a little worried about the behavior differences between Object.keys and for ... in as illustrated by the MDN suggested polyfill. Most of it seems to be related to some bug where for ... in misses some properties that should have been enumerated though. It probably won't affect us.

My vote goes in favor of the Object.keys branch.

If we're to branch, would it make sense to do the check once only?

var keys = typeof Object.keys === 'function' ? Object.keys : function(strMap) {…};

⚡ Although I'm not sure if I've got the formatting right...

Mocha runs the tests in parallel

I don't think that's the case. The problem is that Node itself uses Object.keys, so deleting it can cause problems. For example:

> delete Object.keys true > console.log({}) TypeError: Object.keys is not a function

I don't have a good solution to this problem. The simplest option is to remove the "fast" path, but I'm open to other suggestions. :)

I'd rather remove the slow path tbh.

If you really want to test this branch and don't mind restricting your test suite to run in Node only, you could potentially execute the test in a new VM context.

vm.runInNewContext('console.log(Object.keys)', R.dissocPath(['Object', 'keys'], this))

That said, how desirable is ES3 support?

That said, how desirable is ES3 support?

Dropping support for ES3 seems popular. I'm happy to do so, though I'd like to add a section to the readme to document which environments we target. The description could be as brief as Sanctuary is designed to work in Node and in ES5-compatible browsers.

svozza · 2016-05-09T20:29:38Z

I can see where you're coming from with the sorted keys but to me I think if you care about how the elements line up you're far more likely to work on them as pairs, as per your second example and probably more like this:

const f = (pair) => [R.toUpper(pair[0]), R.inc(pair[1])];

map(f, toPairs({x: 1, y: 2}));

I have a slight preference for leaving the resulting arrays unsorted as iterating over the list three times (if you include the hasOwnProperty check) seems like overkill but I'm happy to go with the consensus.

rjmk · 2016-05-09T23:28:51Z

On the one hand, I think that it would be nice if Sanctuary avoided non-deterministic functions. On the other hand, it seems odd for Sanctuary to impose arbitrary structure on values. I think I like the Set / Bag solution most. I think if people want the values ordered by the keys, but don't want to map over pairs it's not unreasonable to expect them to hand-sort the result of keys and then map that over the object

davidchambers · 2016-05-10T20:16:48Z

index.js

+        for (var idx = 0; idx < ks.length; idx += 1) {
+          vs.push(strMap[ks[idx]]);
+        }
+        return vs;


Since we're thinking about performance, I wonder whether reusing the keys array would help:

var xs = keys(strMap); for (var idx = 0; idx < xs.length; idx += 1) xs[idx] = strMap[xs[idx]]; return xs;

Good idea! ⚡

davidchambers · 2016-05-18T06:15:56Z

S.toPairs added!

Very exciting! I'm not sure about the name, though. In Data.HashMap the three functions are named keys, elems, and assocs/toList (I don't know why there are two names for this one).

Since all the Sanctuary functions live in a single namespace S.toArray is not an option. I like S.pairs, though, rather than than S.toPairs. It matches S.keys and S.values (which are not S.toKeys and S.toValues after all). What do you think?

svozza · 2016-05-18T06:17:57Z

Yep, I prefer S.pairs, the only reason I had it the other way was because I was following Ramda.

davidchambers · 2016-05-18T06:20:08Z

index.js

@@ -2746,6 +2748,59 @@
        return filter(is(type), Just(x));
      });

+  //# keys :: StrMap a -> Array String
+  //.
+  //. Returns an array of the keys of the supplied StrMap.


We could omit "an array of" as this is indicated by the type signature.

Let's append "in arbitrary order".

svozza · 2016-05-18T07:37:42Z

⚡ Think I've everything except adding .sort() to the docs, see #214 (comment).

davidchambers · 2016-05-18T17:10:12Z

index.js

+      function(strMap) {
+        var xs = Object.keys(strMap);
+        for (var idx = 0; idx < xs.length; idx += 1) xs[idx] = strMap[xs[idx]];
+        return xs;


How about using Array#map here too?

return Object.keys(strMap).map(function(k) { return strMap[k]; });

Ha. We've come full circle to my original implementation!

Yep! Now that we're embracing ES5 we can take advantage of its conveniences. :)

Yup, all good!

davidchambers · 2016-05-18T17:51:55Z

🌳 Very nice pull request, Stefano!

davidchambers · 2016-05-31T06:23:21Z

index.js

+        return Object.keys(strMap).map(function(key) { return strMap[key]; });
+      });
+
+  //# toPairs :: StrMap a -> Array (Pair String a)


s/toPairs/pairs/

I'll fix this shortly.

davidchambers mentioned this pull request May 9, 2016

types: define Pair type constructor sanctuary-js/sanctuary-def#68

Merged

svozza force-pushed the keys-values-fn branch 2 times, most recently from f984324 to 7070ee8 Compare May 9, 2016 15:26

davidchambers reviewed May 9, 2016
View reviewed changes

svozza force-pushed the keys-values-fn branch from 7070ee8 to 84f8bdb Compare May 9, 2016 20:21

svozza force-pushed the keys-values-fn branch from 84f8bdb to 8775d66 Compare May 10, 2016 19:59

davidchambers reviewed May 10, 2016
View reviewed changes

svozza force-pushed the keys-values-fn branch 4 times, most recently from ef9e4b0 to 9ce4752 Compare May 11, 2016 21:28

This was referenced May 12, 2016

Add ES3 test for R.keys ramda/ramda#1767

Closed

v0.6.0 release notes sanctuary-js/sanctuary-def#72

Closed

davidchambers mentioned this pull request May 17, 2016

v0.11.0 checklist #202

Closed

12 tasks

svozza force-pushed the keys-values-fn branch from 9ce4752 to f1d6e54 Compare May 18, 2016 05:41

davidchambers reviewed May 18, 2016
View reviewed changes

svozza force-pushed the keys-values-fn branch from f1d6e54 to 5379773 Compare May 18, 2016 07:37

svozza force-pushed the keys-values-fn branch 4 times, most recently from 4490c12 to d97ec53 Compare May 18, 2016 07:47

svozza changed the title ~~Add S.keys, S.values and S.toPairs~~ Add S.keys, S.values and S.pairs May 18, 2016

svozza force-pushed the keys-values-fn branch from d97ec53 to 4bc304d Compare May 18, 2016 12:02

davidchambers reviewed May 18, 2016
View reviewed changes

add S.keys, S.values and S.pairs

7d636b4

svozza force-pushed the keys-values-fn branch from 4bc304d to 7d636b4 Compare May 18, 2016 17:40

davidchambers merged commit bc77ea6 into sanctuary-js:master May 18, 2016

davidchambers reviewed May 31, 2016
View reviewed changes

davidchambers mentioned this pull request May 31, 2016

v0.11.1 release notes #227

Closed

davidchambers mentioned this pull request Jun 26, 2016

define $.Function and $.UnaryTypeVariable sanctuary-js/sanctuary-def#79

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add S.keys, S.values and S.pairs #214

Add S.keys, S.values and S.pairs #214

svozza commented May 8, 2016

davidchambers commented May 9, 2016

kedashoe commented May 9, 2016

davidchambers commented May 9, 2016

kedashoe commented May 9, 2016

davidchambers commented May 9, 2016

svozza commented May 9, 2016 •

edited

Loading

davidchambers commented May 9, 2016

davidchambers May 9, 2016

svozza May 9, 2016 •

edited

Loading

Avaq May 10, 2016

davidchambers May 10, 2016

svozza May 10, 2016

davidchambers May 10, 2016

svozza May 10, 2016

scott-christopher May 10, 2016

davidchambers May 10, 2016

svozza May 11, 2016

svozza commented May 9, 2016 •

edited

Loading

rjmk commented May 9, 2016 •

edited

Loading

davidchambers May 10, 2016

svozza May 11, 2016

davidchambers commented May 18, 2016

svozza commented May 18, 2016

davidchambers May 18, 2016

svozza commented May 18, 2016 •

edited

Loading

davidchambers May 18, 2016

svozza May 18, 2016

davidchambers May 18, 2016

svozza May 18, 2016

davidchambers commented May 18, 2016

davidchambers May 31, 2016

davidchambers May 31, 2016

Add S.keys, S.values and S.pairs #214

Add S.keys, S.values and S.pairs #214

Conversation

svozza commented May 8, 2016

davidchambers commented May 9, 2016

kedashoe commented May 9, 2016

davidchambers commented May 9, 2016

kedashoe commented May 9, 2016

davidchambers commented May 9, 2016

svozza commented May 9, 2016 • edited Loading

davidchambers commented May 9, 2016

Choose a reason for hiding this comment

svozza May 9, 2016 • edited Loading

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

svozza commented May 9, 2016 • edited Loading

rjmk commented May 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchambers commented May 18, 2016

svozza commented May 18, 2016

Choose a reason for hiding this comment

svozza commented May 18, 2016 • edited Loading

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 May 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svozza commented May 9, 2016 •

edited

Loading

svozza May 9, 2016 •

edited

Loading

svozza commented May 9, 2016 •

edited

Loading

rjmk commented May 9, 2016 •

edited

Loading

svozza commented May 18, 2016 •

edited

Loading