-
Notifications
You must be signed in to change notification settings - Fork 117
Finish @manrueda's omitPath #231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Finish @manrueda's omitPath #231
Conversation
I re-generated the index.html because this was the easiest and most reliable way to solve the merge conflicts. The tests of the new function don't actually pass, but I'll solve that after addressing @joshuacc's review comments.
…ntcloud#177 Also reimplements the function as suggested by @joshuacc. More tests pass in this way, but still not all. This is somewhat inconsistent; arguably, _.omitWhen, _.pickWhen and _.selectKeys belong in object.builders, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I'm on the fence about the string to path conversion, but no need to hold this PR up.
Please clarify, what exactly are you on the fence about, regarding the string to path conversion? |
@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:
Please let me know what you think. |
Slight possible sophistication:
|
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. |
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:
Instead of:
|
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
No problem, I'll just submit a PR when the time comes and then ask for your input again.
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 I'll prepare a pull request, it's probably easier in that way. |
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:
_.omitPath
function was moved from the object.selectors to the object.builders category._.snapshot
._.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.This feature reveals some issues that will require modularization (#220) as well as some breaking changes further down the road to fix:
_.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 internalkeysFromPath
parser because it is in theobject.selectors
module. It uses a simplerstring.split('.')
instead, which supportsa.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
.