Skip to content

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

Merged
merged 6 commits into from
Sep 17, 2020

Conversation

jgonggrijp
Copy link
Contributor

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.

manrueda and others added 6 commits September 21, 2014 16:22
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.
@jgonggrijp jgonggrijp requested a review from joshuacc August 31, 2020 13:14
Copy link
Contributor

@joshuacc joshuacc left a 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.

@jgonggrijp
Copy link
Contributor Author

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

@jgonggrijp
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

@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.

@jgonggrijp jgonggrijp merged commit 8d7d82a into documentcloud:master Sep 17, 2020
@jgonggrijp jgonggrijp deleted the manrueda-omitPath branch September 17, 2020 13:22
jgonggrijp added a commit to jgonggrijp/underscore that referenced this pull request Sep 21, 2020
jgonggrijp added a commit to jgonggrijp/underscore that referenced this pull request Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants