Finish @manrueda's omitPath

[jgonggrijp]

This is a continuation of #177 by @manrueda, processing the review comments by @joshuacc. This is probably easiest to review by approaching it as a completely new PR, but for completeness' sake, I hereby list the differences from the original PR:

• Markdown formatting in the documentation was corrected.
• The added _.omitPath function was moved from the object.selectors to the object.builders category.
• The implementation was simplified considerably by building on _.snapshot.
• Path semantics were made consistent with the rest of Underscore and Contrib. The original implementation would branch out over arrays, so that _.omitPath([{a: 1}, {a: 2}], 'a') would return [{}, {}] (or at least, that was the intention). The current PR instead treats arrays and objects in the same way: _.omitPath([{a: 1}, {a: 2}], '0.a') returns [{}, {a: 2}] while _.omitPath([{a: 1}, {a: 2}], 'a') just returns an identical copy.
• Indentation was added to the tests to make them easier to read.
• The implementation actually works (none of the tests passed in the original PR).

This feature reveals some issues that will require modularization (#220) as well as some breaking changes further down the road to fix:

• Like _.omitPath, _.omitWhen, _.pickWhen and _.selectKeys (which is just _.pick) should probably be in the object.builders category.
• _.omitPath permits string paths, but it cannot use the internal keysFromPath parser because it is in the object.selectors module. It uses a simpler string.split('.') instead, which supports a.0 but not ['a']['0'].

I also think converting strings to paths should be opt-in, because object keys can be arbitrary strings that might look like a multi-faceted path in principle (and there is no path syntax that can avoid this problem). Removing the implicit string-to-path conversion from all functions that do this would be a big breaking change, but if you agree with such a future step, I could leave out the string.split from the current PR so that at least users don't start depending on it in _.omitPath.

[joshuacc]

This looks good to me. I'm on the fence about the string to path conversion, but no need to hold this PR up.

[jgonggrijp]

Please clarify, what exactly are you on the fence about, regarding the string to path conversion?

[jgonggrijp]

@joshuacc I've been thinking about how to make path strings an opt-in feature, preferably in a way that makes the behavior consistent accross the union of all Underscore and Contrib functions, without hindering modularity. I came up with the following:

• Underscore's internal _deepGet (which is exclusively array-based) becomes a public _.get function.
• All Underscore and Contrib functions that do path-based deep property extraction are rewritten to use _.get.
• Contrib's internal keysFromPath becomes a public _.stringToPath function.
• Users can opt in to string-based paths for all deep path-extracting functions at once by overriding _.get = _.compose(_.get, _.stringToPath).

Please let me know what you think.

[jgonggrijp]

Slight possible sophistication:

• Underscore also gets a public _.toPath function, which by default just puts a string in a one-element array and returns arguments that are already arrays unmodified.
• _.get always takes its argument through _.toPath to ensure that its argument is an array.
• Users override _.toPath = _.stringToPath instead of overriding _.get itself.

[joshuacc]

To clarify my earlier comment, I was considering whether this method should be consistent with others that allow path strings. Given that changing the behavior of all of them is on the table, I've decided that it probably doesn't matter all that much which route we take there.

Your other comments look good, but I don't have a thorough response to them right now.

[jashkenas]

Since my comments were requested — I’m not a fan of string-to-path conversions in JavaScript, and would prefer that they remain kept out of Underscore, although I think they're totally fair game for Contrib.

In general, I prefer explicit accessor functions. So:

_.something(collection, (item) => item.property[2].key)

Instead of:

_.something(collection, "property.2.key")

[jgonggrijp]

@joshuacc

To clarify my earlier comment, I was considering whether this method should be consistent with others that allow path strings. Given that changing the behavior of all of them is on the table, I've decided that it probably doesn't matter all that much which route we take there.

For what it's worth, I agree about the consistency, but it just isn't possible with the current architecture of Contrib (unless you put all path-dereferencing functions in a single module). The modularization should fix that, together with the _.get/_.toPath construction I'm proposing. That should actually raise the bar of consistency a bit, i.e., not only within Contrib but also between Underscore and Contrib.

Your other comments look good, but I don't have a thorough response to them right now.

No problem, I'll just submit a PR when the time comes and then ask for your input again.

@jashkenas

Since my comments were requested — I’m not a fan of string-to-path conversions in JavaScript, and would prefer that they remain kept out of Underscore, although I think they're totally fair game for Contrib.

I'm not surprised and in fact, I agree. I'm not proposing to make string paths a part of Underscore. Rather, I'm proposing to add a customization point, similar to _.iteratee, so that users can enable string paths from the outside (like regex iteratees). And to provide a ready-made override in Contrib so that users who want string paths don't have to implement it by themselves.

I'll prepare a pull request, it's probably easier in that way.