Skip to content
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

_.islike(obj, pattern) tests objects are like patterns #196

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

joebain
Copy link

@joebain joebain commented Jun 19, 2015

The impetus behind this function is to check arguments received are in the expected format and be able to show an error or do other appropriate actions if not.

_.islike(
  {name: "James", age: 10, hobbies: ["football", "computer games", "baking"]},
  {name: "", age: 0, hobbies: [""]}
)

Please check the docs for a more thorough description and let me know what you think.

}

if (typeof obj !== typeof pattern) return false;
if (pattern instanceof Array && !(obj instanceof Array)) return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I would suggest _.isArray if you keep this check, but another alternative is patter.constructor !== obj.constructor

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_.isArray is a good call. I don't think comparing constructors works because I'm only trying to reject arrays at this point.

Functionaly the same but using more underscore-ish style and making
use of underscore methods for type checking where it makes sense.

Also updated the documentation for _.islike to be clearer and better
explain arrays and nested objects.
@jgonggrijp jgonggrijp added before modules This needs to be tackled before modularization (temporary label, see #220) enhancement labels Aug 3, 2020
Copy link
Contributor

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a really interesting submission, but I have some concerns.

Most importantly, arrays and objects are treated very differently, which I think makes it structurally inconsistent and incomplete. In arrays, the available types are independent of the key (index) and no recursion is performed. In objects, each key has a separate type and recursion is performed. There are several common situations that are handled either poorly or not at all.

Union types: consider an object where a particular key may be either string or number but nothing else.

_.islike({a: 20}, {a: ?}) // true
_.islike({a: 'x'}, {a: ?}) // true
_.islike({a: true}, {a: ?}) // false

Incidentally, this also applies to isolated values (_.islike(20, ?)). With the current implementation, array elements are the only place where union types are supported.

Dictionary types: analogous to arrays in the current implementation, the key may not always matter in objects, either. Sometimes you just want an object with arbitrary keys and you want to constrain the value type for all of them, for example a dictionary of strings.

_.islike({a: 'x'}, {?: ''}) // true
_.islike({b: 'y', c: 'z'}, {?: ''}) // true
_.islike({d, 'foo', e: 20}, {?: ''}) // false

Tuple types: conversely, in arrays it sometimes matters which type occurs at which index. For example, when building a dictionary of numbers using _.object, you want each entry to be represented by an array where the first element is a string and the second is a number.

_.islike(['a', 1], ??) // true
_.islike(['a', 'b'], ??) // false
_.islike(['a', 'b', 1], ??) // false

With the current implementation, tuples can be emulated using the object notation, but this also means a radical change compared to homogeneous arrays, as it adds recursion and removes the posibility of a union type.

_.islike(['a', 1], {0: '', 1: 0, length: 0}) // true
_.islike(['a', 'b'], {0: '', 1: 0, length: 0}) // false
_.islike(['a', 'b', 1], {0: '', 1: 0, length: 0}) // false

Another thing that concerns me is the use of primitive literals in patterns, which are checked using typeof, as well as the need to pass constructors for common non-primitive types, which are then checked using instanceof. There are two main problems with that.

Firstly, primitive values and their object-wrapped counterparts are not interchangeable:

_.islike(1, 0) // true
_.islike(1, Number) // false
_.islike(Object(1), Number) // true
_.islike(Object(1), 0) // false

Secondly, instanceof doesn't work if you got an object from another frame or script context.

// someDate came from another frame
_.islike(someDate, Date) // false

This could be made much more robust by allowing to use string names of types as patterns, for example 'Number' or 'Date', and then using those names to look up the corresponding _.isType function from Underscore in the implementation.

// someDate came from another frame
_.islike(someDate, 'Date') // _.isDate(someDate): true

The use of string names for types would have the added benefit that people can add their own typechecks, simply by mixing additional isType functions into Underscore. Also, even when there is no matching isType function, the implementation could still fall back to using Object.prototype.toString:

_.islike(x, 'Foo') // Object.prototype.toString.call(x) === '[object Foo]'

Of course, this mechanism doesn't have to completely replace the existing logic. You could continue to allow literal numbers, booleans, null, undefined and empty strings by internally substituting the type names 'Number', 'Boolean', 'Null', 'Undefined' and 'String', respectively. You could also still allow passing a constructor instead of a type name and check it using instanceof.

In conclusion, I think this would be a great addition to Contrib, but it would require a serious redesign in order to meet our quality standards. In summary:

  • Make arrays and objects either both recursive or both nonrecursive.
  • Allow type unions anywhere, not just in array elements.
  • (Not mentioned above, optional) support for matching "anything" or "anything but" (top and complement types).
  • (Also not mentioned above and optional) support for a "never" (bottom) type, which could e.g. be used to signify that a particular object key should be absent.
  • Allow order-sensitive array checking (tuple types).
  • Allow key-agnostic object checking (dictionary types).
  • Use more robust atomic type checking based on Underscore's isType functions.

Since there is considerable work to do, I'm going to change this into a draft. I suggest postponing the work until after modularization (see #220). In the meanwhile, we can continue to discuss design choices in here.

Comment on lines +23 to +26
if (typeof obj !== typeof pattern) return false;
if (_.isArray(pattern) && !_.isArray(obj)) return false;

var type = typeof pattern;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is going astray here, that's something that needs fixing before we merge this.

Comment on lines +18 to +19
var islike = function(obj, pattern) {
if (typeof pattern === "function") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, while I personally prefer 4-space indents, the present convention in Underscore and Contrib is 2-space indents, so that's something that needs fixing as well.

var type = typeof pattern;

if (type == "object") {
if (pattern instanceof Array) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use _.isArray instead, because the pattern might have used an Array constructor from a different frame or script context.

@jgonggrijp jgonggrijp marked this pull request as draft August 28, 2020 14:57
@jgonggrijp jgonggrijp added after modules This should be postponed until after modularization (temporary label, see #220) question and removed before modules This needs to be tackled before modularization (temporary label, see #220) labels Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
after modules This should be postponed until after modularization (temporary label, see #220) enhancement question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants