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

Add fix suggestions when accessing a nonexistent property in proxy mode #782

Merged
merged 1 commit into from
Sep 8, 2016

Conversation

not-an-aardvark
Copy link
Contributor

@not-an-aardvark not-an-aardvark commented Sep 4, 2016

fixes #771

As suggested in #771, when a nonexistent property is accessed in proxy mode, this computes the levenshtein distance to all possible properties in order to suggest the best fix to the user.

chai.expect(false).to.be.fals; // Error: Invalid Chai property: fals. Did you mean "false"?
chai.expect('foo').to.be.undefind; // Error: Invalid Chai property: undefind. Did you mean "undefined"?

I decided to not suggest a fix if the closest property has a string distance of 4 or more, simply because the suggestion probably will not be very useful at that point.

chai.expect('foo').to.be.fdsakfdsafsagsadgagsdfasf // error thrown, no fix suggested

@keithamus
Copy link
Member

Great work here @not-an-aardvark! LGTM. This may make you the coolest non-aardvark I've met!

@meeber @lucasfcosta thoughts?

@keithamus keithamus modified the milestone: 4.0 Sep 6, 2016
@lucasfcosta
Copy link
Member

lucasfcosta commented Sep 6, 2016

Hi @not-an-aardvark, thanks for your contribution, I'd like to frame this code and hang it on my wall! It looks really elegant! 😄
Whenever I see an intelligent recursion such as this one my heart beats faster.

I've gotta confess I needed half an hour to truly understand how that works, but it was totally worth it.

Just in case anyone else in the future also needs some help with this, take a look at this link, it's an excellent learning resource.

LGTM! I'm not sure if I should merge this or not since @keithamus has tagged @meeber too.
Anyway, it's always good to hear @meeber's opinion 😄

@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Sep 6, 2016

I'd like to frame this code and hang it on my wall! It looks really elegant! 😄

Thanks!

Just in case anyone else in the future also needs some help with this, take a look at this link, it's an excellent learning resource.

In addition, I would also recommend this Wikipedia article on Levenshtein distances. The implementation in this PR is based on the article's definition here.

@meeber
Copy link
Contributor

meeber commented Sep 6, 2016

Please don't merge yet, I need to explore a few things with this.

@meeber
Copy link
Contributor

meeber commented Sep 8, 2016

This looks pretty neat. But I think we should reduce the suggestion list to Chai keywords only.

If we change getProperties(target) to Object.getOwnPropertyNames(Object.getPrototypeOf(target)), then the following suggestions from Object.prototype get removed:

<   '__defineGetter__',
<   '__defineSetter__',
<   '__flags',
<   '__lookupGetter__',
<   '__lookupSetter__',
<   '__proto__',
<   'hasOwnProperty',
<   'isPrototypeOf',
<   'propertyIsEnumerable',
<   'should',
<   'toLocaleString',
<   'toString',
<   'valueOf',

That leaves the following list with three needing to be manually filtered out:

[ 'Arguments',
  'NaN',
  'Throw',
  '__methods',             # Manually filter out; internal use
  '_obj',                  # Manually filter out; internal use
  'a',
  'above',
  'all',
  'an',
  'and',
  'any',
  'approximately',
  'arguments',
  'assert',                # Manually filter out; internal use
  'at',
  'be',
  'been',
  'below',
  'but',
  'by',
  'change',
  'changes',
  'closeTo',
  'constructor',
  'contain',
  'contains',
  'decrease',
  'decreases',
  'deep',
  'does',
  'empty',
  'eq',
  'eql',
  'eqls',
  'equal',
  'equals',
  'exist',
  'extensible',
  'false',
  'finite',
  'frozen',
  'greaterThan',
  'gt',
  'gte',
  'has',
  'have',
  'haveOwnProperty',
  'haveOwnPropertyDescriptor',
  'include',
  'includes',
  'increase',
  'increases',
  'instanceOf',
  'instanceof',
  'is',
  'itself',
  'key',
  'keys',
  'least',
  'length',
  'lengthOf',
  'lessThan',
  'lt',
  'lte',
  'match',
  'matches',
  'members',
  'most',
  'nested',
  'not',
  'null',
  'of',
  'ok',
  'oneOf',
  'ordered',
  'ownProperty',
  'ownPropertyDescriptor',
  'property',
  'respondTo',
  'respondsTo',
  'same',
  'satisfies',
  'satisfy',
  'sealed',
  'string',
  'that',
  'throw',
  'throws',
  'to',
  'true',
  'undefined',
  'which',
  'with',
  'within' ]

@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Sep 8, 2016

If we change getProperties(target) to Object.getOwnPropertyNames(Object.getPrototypeOf(target))

That works, but it will cause issues if someone tries to subclass chai.Assertion. (I'm not sure whether this is a supported use-case.)

As an alternative, we could specifically exclude keys in Object.prototype:

getProperties(target).filter(function (property) {
  return !Object.prototype.hasOwnProperty(property) && ['__flags', '__methods', '_obj', 'assert'].indexOf(property) === -1;
});

@meeber
Copy link
Contributor

meeber commented Sep 8, 2016

@not-an-aardvark Alternative looks good.

@not-an-aardvark
Copy link
Contributor Author

Updated to use the alternative version, and added a couple tests for it.

@meeber
Copy link
Contributor

meeber commented Sep 8, 2016

@not-an-aardvark LGTM! Thanks for doing this :D

Let's get one more LGTM before merging since there were changes. @keithamus @lucasfcosta?

@lucasfcosta
Copy link
Member

LGTM too!
Great catch meeber, I haven't thought about it, but now it makes total sense.

@lucasfcosta lucasfcosta merged commit e5e6f5c into chaijs:master Sep 8, 2016
@not-an-aardvark not-an-aardvark deleted the did-you-mean branch January 25, 2017 17:25
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.

Use levenshtein distance to suggest alternatives in proxy mode
4 participants