remove R.set

[davidchambers]

R.set was added in #1088; R.merge was deprecated in #1105. This pull request reverts these two changes, for three reasons:

• @nthtran drew attention to the surprising behaviour of R.set({}) in Add deepMerge/set/extend/mixin #1088 (comment). @paldepind opened Handle empty objects and falsey values in set #1170 to address this, but we've yet to reach consensus on the desired behaviour.
• We failed to consider R.mergeAll when we decided to deprecate R.merge. It's inconsistent to have the former without the latter. Determining the correct course of action will require thought.
• I'd like to release v0.15.0, which is currently blocked while we decide how R.set should handle {}. Reverting these changes will allow us to release v0.15.0, then devote time to the above concerns.

[buzzdecafe]

👍

[CrossEye]

🌿

[paldepind]

It sounds to me like you've forgotten how broke merge is.

I'd personally much rather have the current set than the current merge. The issues with the current merge are much bigger than the potential issues with the current set. merge has its arguments in the wrong order and it is not recursive. Those issues are much more real and limiting in what one can do. They are not edge cases – they are fundamental problems and inconsistencies (and shallowness is a bigger inconsistency in the new release since R.equals and R.evolve are now recursive).

The discussion of the "issues" regarding the current set implementation has been completely abstract. I think we would learn more about what behavior is actually most useful in the real world by letting people start using the function.

[CrossEye]

I have been sceptical all along of the deep merge. Not that I think it's a bad idea, but that I expect odd difficulties and strange corner cases. And I also believe there is an important place for shallow merges.

We have had merge for a long time. It was created, as mixin back in #21, and was similar to the extend functions of Underscore, lodash, or jQuery, but only partially similar: It never modified its target object, it didn't offer a deep option, and it only accepted one source object. Because of the latter, we, I guess, had the choice of ordering the parameters in either way we chose: source first or target first. But I don't know that we ever considered switching from the order that had already been established.

Based on #777 and #778, we renamed this to merge in #783. In retrospect, I'm not sure this was such a good idea, as it seems to have lead to an expectation that this function should be doing something that I, at least, never expected it to do.

My reasons for wanting this function had to do with cases like this:

    var process = function() {
      var defaults = {
        prop1: 'val1'
        prop2: 'val2',
        //...
      };
      return function(param, opts) {
        var options = R.mixin(defaults, opts || {});
        // continue processing using options;
      };
    }());

So the parameter order for this doesn't feel wrong for me, the general functionality of the shallow version doesn't feel wrong. The notion of a deep merge was more interesting as a theoretical extension of assoc than as a construct that I've ever needed. And, as I said, I've been suspicious about how well it can be implemented (although I know that lodash has managed to do a fairly credible job of it.)

So, I'm for backing off these changes in order to get 0.15 out and figure them out afterward.

[paldepind]

Not that I think it's a bad idea, but that I expect odd difficulties and strange corner cases.

Just because a thing can be implemented in two different ways that are both reasonable doesn't make one version "odd" or "strange". I think that both the behavior of the current merge and the one in my PR makes sense and are explainable. Just in different ways.

So the parameter order for this doesn't feel wrong for me, the general functionality of the shallow version doesn't feel wrong.

In general all Ramda functions that returns a modified thing takes the thing to update as it's last argument and the form of change as its first argument. You are merging opts on top of defaults and thus defaults is the base, the thing that is modified. At least to me. And then there is he symmetry with assoc.

The notion of a deep merge was more interesting as a theoretical extension of assoc than as a construct that I've ever needed.

Deep merge has been requested several times by users. It is desired. set can do everything merge can. It's a superset in functionality.

How is not releasing set going to give us any additional insights? Let me point out that merge will still be a part of the next release in any case. If anybody runs in to issues with set in the fringe cases they can keep using merge and they can share their practical experiences so we will be making any potential changes to set based on practical experience instead of theoretical discussions.

[davidchambers]

@paldepind, this pull request in no way reflects the relative merits of R.merge and R.set. I did and still do find your arguments for R.set compelling. But before we include R.set in a release we should decide whether or not to merge #1170, and also what to do with R.mergeAll.

I suggest that we merge this pull request, publish v0.15.0, then focus our attention on R.set and R.mergeAll. Ideally we would follow v0.15.0 with v0.16.0 just a day or two later. This would make R.set available just as soon as it would be if we were to delay v0.15.0, but has the advantage of making all the other v0.15.0 features available right away.

[paldepind]

But before we include R.set in a release we should decide whether or not to merge #1170, and also what to do with R.mergeAll.

I personally don't care the slightest about #1170. I made the change because some users did. Anyway, I do not want to hold back 0.15 and if we aim for a quick 0.16 release then this is not an issue.

Feel free to go ahead 😸

[davidchambers]

[I]f we aim for a quick 0.16 release then this is not an issue.

Great. Let's aim to release v0.16.0 next weekend (or sooner). I'll merge this pull request, publish v0.15.0, and write the upgrade guide. :)