strmap: add S.insert and S.remove

[samueller]

As discussed in issue #434.

[gabejohnson]

We may want to consider adding delete and update as well while we're at it.

[davidchambers]

Lovely pull request, @samueller! I see that you took care to follow the project's conventions. 🎉

We may want to consider adding delete and update as well while we're at it.

I agree. I'd been thinking delete would be a natural inclusion in this pull request. update is new to me, but it's closely related to the other two functions.

Scott, please add delete to this pull request. You're welcome to add update too, but I don't mind leaving that for another pull request.

[davidchambers]

Let's avoid using the term property in relation to string maps. The fact that string maps are unadorned objects is an implementation detail in most contexts.

Adding the following would help those coming from Ramda who are likely to expect the function to be named assoc:

Equivalent to Haskell's insert function. Similar to Clojure's assoc function.

[samueller]

Makes sense. I'll remove "property" and add your 2 sentences.

[gabejohnson]

@davidchambers, @samueller I'm rethinking update. It should probably go in a separate PR with alter.

They both rely on Maybe whereas insert and delete do not.

alter can be implemented in terms of insert and delete and update in terms of alter.

[davidchambers]

It should probably go in a separate PR with alter.

Okay. insert and delete should still be inserted into the project atomically. :)

[samueller]

@davidchambers, @gabejohnson. So I'll add delete to this PR and nothing else, correct?

[davidchambers]

So I'll add delete to this PR and nothing else, correct?

👍

[samueller]

I created the S.delete function and all the tests pass, but ESLint is reporting an error when I run make test lint:

/Users/scott/Documents/code/sanctuary/index.js
  3754:5  error  .delete is a syntax error  dot-notation

✖ 1 problem (1 error, 0 warnings)

make: *** [lint] Error 1

I'm guessing this is a problem because delete is a keyword in JavaScript. Do we modify the ESLint configuration or change the name of this function?

[gabejohnson]

We have to change it. Calling it delete will mean that no one will be able to use it by that name unqualified.

We could call it remove, deleteFrom, deleteEntry, or what have you.

I apologize. This should have occurred to me before I was suggesting it.

[samueller]

I like remove.

[davidchambers]

s/functiom/function/

[samueller]

good catch!

[davidchambers]

This is unsafe due to inherited properties. 'toString' in {}, for example, evaluates to true.

Is this implementation suitable?

var result = Z.concat(strMap, {});
delete result[key];
return result;

[samueller]

Wasn't aware of the inherited properties issue. The initial thought was to use concat in the way you wrote. But it bothered me to add a property to an object and then delete it. In addition, I liked the efficiency of Haskell's delete function where it returns the original Map if the key didn't exist.

What about if (strMap.hasOwnProperty(key))?

[davidchambers]

What about if (strMap.hasOwnProperty(key))?

This would get us a bit closer, but we must allow for the possibility of 'hasOwnProperty' being one of the keys. We'd need Object.prototype.hasOwnProperty.call(strMap, key).

I think we should match the semantics of Z.concat, though, which is interested in enumerable properties rather than own properties. For example:

function Point() {}
Point.prototype.x = 0;
Point.prototype.y = 0;

Z.concat(new Point(), {});  // => {x: 0, y: 0}

[samueller]

Good catch on the 'hasOwnProperty' as a key issue.

I understand Z.concat uses for..in to iterate through keys of enumerable and inherited enumerable properties. We could useObject.prototype.propertyIsEnumerable.call(strMap, key), but that will only tell us if a property is enumerable. It will return false if a property is inherited enumerable as in your Point example above. So, if we want to stick with returning the original string map when it doesn't contain the key to remove, we need to iterate through the keys before determining whether we can simply return the original string map:

  function remove(key, strMap) {
    var found = false;
    var result = {};
    for (var k in strMap) {
      if (k === key) {
        found = true;
      } else {
        result[k] = strMap[k];
      }
    }
    if (found) {
      return result;
    } else {
      return strMap;
    }
  }

If optimizing on removing non-existing keys more often, we could search for the key first before copying.

[davidchambers]

if (found) {
  return result;
} else {
  return strMap;
}

If we're to do the work to generate result, why not return it in all cases? Sanctuary users, ideally, will not care about object identity, so it shouldn't be necessary to have S.remove(key, strMap) === strMap evaluate to true in certain cases. It may, though, be useful to guarantee that this expression never evaluates to true (the delete-based implementation above takes advantage of the fact that Z.concat always returns a new string map).

[davidchambers]

Let's use this implementation:

var result = Z.concat(strMap, {});
delete result[key];
return result;

[The implementation above] is automatically updated when/if Z.concat is updated.

I consider this a requirement. I'm keen to avoid inconsistencies such as the one documented in ramda/ramda#1941.

[kedashoe]

We also have S.keys. Whichever way we go with properties to include, I think we should have (pseudo code, I think I get to the idea):

function insert_test(key, val, strMap) {
  new Set(S.keys(strMap).concat([key])) === new Set(S.keys(S.insert(key, val, strMap)))
}

?

[davidchambers]

Great point, Kevin! It would be good to test this property. Here are a few more:

• S.insert (key) (val) (S.insert (key) (val) (map)) = S.insert (key) (val) (map)
• S.remove (key) (S.remove (key) (map)) = S.remove (key) (map)
• S.sort (S.keys (map)) = S.sort (S.keys (S.map (S.I) (map)))

[samueller]

I like the tests and have added them, except the last assertion you mentioned @davidchambers. I assume that test was meant for the keys or the map function? I can add that to this pull request if you want.

[davidchambers]

I assume that test was meant for the keys or the map function?

Yes, you're right. It would assert that S.map does not "promote" any properties which S.keys does not see. Let's leave that for a separate pull request.

I look forward to seeing your changes. Feel free to push the changes you have made so far even if there are more to come. You can tidy the commit history before we merge the pull request. :)

[davidchambers]

Even if we decide to return the input in some cases, I don't think this should be enforced by the test suite. If, on the other hand, we decide it's important to return a new string map in all cases we could consider asserting that the output is not identical to the input.

[samueller]

If we're specifying that remove returns the original object, shouldn't we test for it? We could just not specify whether removing a non-existing key will return the original object. Or we could always return a new string map.

[davidchambers]

Let's drop this example. The following example is sufficient to demonstrate that "removing" a non-existent key is a valid operation.

[samueller]

True

[samueller]

Don't know if there was a notification, but I pushed changes yesterday

[davidchambers]

Don't know if there was a notification, but I pushed changes yesterday

There are notifications for regular pushes but not for --force pushes.

I'll have a look at the changes now. :)

[davidchambers]

The second sentence is no longer accurate.

[samueller]

You are correct, I apologize for the oversight! Pushing an update now.

[davidchambers]

I think we should replace a with the in several places. In the second sentence it's important that we write the key rather than a (any) key.

[samueller]

Makes sense.

[davidchambers]

This might make the double application even clearer:

var f = S.insert(key, val);
eq(f(f(strMap)), f(strMap));

[samueller]

I agree.

[samueller]

Would this be clearer?

var insert = S.insert(key, val);
eq(insert(insert(strMap)), insert(strMap));

I can do the same for remove:

var remove = S.remove(key);
eq(remove(remove(strMap)), remove(strMap));

[davidchambers]

Works for me. 👍

The name doesn't much matter to me, so long as the pattern is clear. :)

[davidchambers]

We should avoid shadowing the global test function provided by Mocha.

[samueller]

Yes. I wasn't aware of the global test function.

[davidchambers]

Here's a less imperative implementation:

function unique(xs) {
  if (xs.length === 0) return [];

  var sorted = S.sort(xs);
  var x = sorted[0];
  return sorted.slice(1).reduce(function(acc, x) {
    return S.equals(x, acc.prev) ? acc : {xs: S.append(x, acc.xs), prev: x};
  }, {xs: [x], prev: x}).xs;
}

I think we should avoid requiring a sorted array as input, even if this means we end up sorting twice in some cases.

[samueller]

What about:

function unique(xs) {
  return S.sort(xs).reduce(function(acc, x, i, xs) {
    return i > 0 && S.equals(x, xs[i-1]) ? acc : S.append(x, acc);
  }, []);
}

or

function appendIfUnique(acc, x, i, xs) {
  return i > 0 && S.equals(x, xs[i-1])
    ? acc
    : S.append(x, acc);
}

function unique(xs) {
  return S.sort(xs).reduce(appendIfUnique, []);
}

[davidchambers]

The use of indexes and the i > 0 guard in particular are the things which bother me. If we're to deal with indexes I'd prefer to use a for loop:

function unique(xs) {
  if (xs.length === 0) return [];

  var sorted = S.sort(xs);
  var prev = sorted[0];
  var result = [prev];
  for (var idx = 1; idx < sorted.length; idx += 1) {
    if (!S.equals(sorted[idx], prev)) result.push(prev = sorted[idx]);
  }
  return result;
}

[samueller]

I can replace with your reduce implementation. Just for my own edification, curious why you don't like indexes and i > 0? Is it the potential for getting the wrong array element?

[davidchambers]

I'd prefer to handle the special case ([]) in one place rather than allow it to complicate the common code path. I don't mind using array indexes when implementing Sanctuary functions, but in contexts in which performance is less of a concern (such as a test suite) I'd prefer to avoid "cheating" (keep in mind that S.map and friends do not expose indexes). In this case we're using an index to access the previous element, but it seems more natural to me to keep track of this.

This is all subjective, and I don't mean to assert that my preferences are better than alternatives in any meaningful way.

[samueller]

Updated pull request.

[davidchambers]

I don't see value in testing so many combinations. Would something like this suffice?

function Point() {}
Point.prototype.x = 0;
Point.prototype.y = 0;

eq(S.remove('x', new Point()), {y: 0});
eq(S.remove('y', new Point()), {x: 0});
eq(S.remove('z', new Point()), {x: 0, y: 0});

[davidchambers]

Let's replace this with a property-based test:

jsc.assert(jsc.forall(jsc.string, jsc.dict(jsc.number), function(key, map) {
  var remove = S.remove(key);
  return S.equals(remove(remove(map)), remove(map));
}), {tests: 1000});

You should be able to do something similar for testDoubleInsert. :)

[samueller]

Was unaware of jsverify! Very nice. I've made the changes as well as the above Point inherited properties test. Note that it does take well over triple the time to run the tests now on my MacBook due to insert.js taking 4.5secs and remove.js taking 3.5secs with these jsverify tests.

[davidchambers]

This means we test the property 1000 times each time we call testInsert, which seems excessive. ;)

Testing the property 1000 times rather than 8000 times may prevent the tests from timing out on CircleCI.

[samueller]

Silly mistake on my part to call testDoubleInsert on every call to testInsert

[davidchambers]

No worries. :)

[davidchambers]

As mentioned above, let's test the property just 1000 times. This will mean we can remove testRemove and make assertions here directly:

eq(S.remove('a', {}), {});
...
eq(S.remove('b', {a: 1, b: 2, c: 3}), {a: 1, c: 3});

I think we could achieve 100% coverage with fewer test cases. These should suffice:

eq(S.remove('x', {x: 1, y: 2}), {y: 2});
eq(S.remove('y', {x: 1, y: 2}), {x: 1});
eq(S.remove('z', {x: 1, y: 2}), {x: 1, y: 2});

[samueller]

I've made those changes locally. Are the insert tests excessive as well?

[davidchambers]

I would say so. I like to be able to look at each test case and see what it alone tests. I see only two distinct scenarios: either the key is already present or it is not. We really only need two unit tests (in addition to the property tests), but I don't mind including a few more if it makes a nice a, b, c or 1, 2, 3 pattern.

[samueller]

Updated pull request with 3 insert tests, making somewhat of an a, b / 1, 2, 3 pattern.

[davidchambers]

Shall we include similar test cases for insert?

[samueller]

Makes sense

[davidchambers]

Let's just include the body of testDoubleInsert here. The code is self-descriptive.

[davidchambers]

Correct me if I'm wrong, but I don't think this tests anything not covered by other tests is this file. If we were to test new Point() the equivalence would not hold because S.keys(new Point()) is [] rather than ['x', 'y']. This suggests that S.keys cannot be implemented in terms of Object.keys. This changes the way I feel about sanctuary-js/sanctuary-type-classes#65. I'll comment on that issue shortly.

[samueller]

I added the S.keys test above because of the conversation between you and @kedashoe earlier. I didn't realize S.keys(new Point()) would yield []. I understand you're going to decide whether to make S.keys get keys of enumerable inherited properties or change S.concat to not get enumerable inherited properties. I can wait until you decide or remove testKeysOfInsert entirely.

[davidchambers]

We'll update the behaviour of Z.concat and S.concat. The behaviour of S.keys will remain as is.

[samueller]

So Z.concat and S.concat no longer include enumerable inherited properties? And I assume then this PR's S.insert and S.remove should also no longer include enumerable inherited properties. Then I'll no longer test for enumerable inherited properties or use new Point(). Or I could explicitly test that enumerable inherited properties are not included in the results of S.insert and S.remove?

Do you want me to keep or delete the testKeysOfInsert code?

[samueller]

@davidchambers just following up on this.

[samueller]

Makes sense. I'll be ready for step 6 :). Am I correct in understanding that Z.concat and S.concat no longer include enumerable inherited properties?

[davidchambers]

Am I correct in understanding that Z.concat and S.concat no longer include enumerable inherited properties?

Just about. Z.concat and S.concat will no longer include enumerable inherited properties. ;)

[davidchambers]

sanctuary-js/sanctuary-type-classes#70

[davidchambers]

@samueller, I've completed steps 1–5 in the list above so you should no longer be blocked. :)

[davidchambers]

#439 introduced a StrMap section, so when you rebase please also update the commit message prefix from object: to strmap:.

[davidchambers]

I'd love to include these additions in the next release. I'm happy to make the final changes if you're busy with other things, @samueller. Let me know.

[davidchambers]

Would you mind having another look at this pull request, @gabejohnson? 👀

[gabejohnson]

I like this implementation in particular 😄

[davidchambers]

I do too. Thanks again, @masaeedu, for adding this handy function. :)

[masaeedu]

Is there a policy of not "cannibalizing" Sanctuary functions to implement other things? If you are allowed to use stuff from S instead of just Z, you could do something like const insert = compose2(concat, singleton) which perhaps saves you the cost of having a wrapping function for just juggling the arguments around. I guess without a type system to guide you, getting too abstract gets confusing.

[gabejohnson]

I think insert`assoc` is used frequently enough to rate it's own function. Unless I'm misunderstanding you.

[masaeedu]

@gabejohnson I agree with having insert as a separate function, I didn't mean to say this shouldn't be part of the library. I'm just saying that when we implement these functions, it seems like we prefer to use Z in the implementation even when it makes the implementation a little more awkward than if we used the equivalent S function.

[gabejohnson]

This is because of the overhead introduced by def (currying, redundant type checking machinery). Some care is taken to write performant implementations.

[davidchambers]

Haha! Your use of italics just made me chuckle, Gabe. :)

We avoid using the curried functions, Asad, for the reason Gabe gave, but the implementation functions can freely be used in other implementations. The head implementation, for example, is defined in terms of the at implementation:

function head(xs) {
  return at(0, xs);
}

[davidchambers]

Thanks for your work on this pull request, @samueller. :)