Refactor `ownProperty` #810

meeber · 2016-09-28T11:15:09Z

This PR is split into two commits. The first commit addresses a small issue with the failed assertion error message being inconsistent across property-related assertions. Sometimes the message was "expected blah to have a property..." and other times it was "expected blah to have property". I settled on the second style because otherwise the logic gets more complex having to deal with changing between "a" or "an" depending on if the next word in the message starts with a vowel (such as "own").

The second commit makes the following changes:

Add own flag
Make ownProperty and haveOwnProperty aliases of own.property
Add deep support to own.property
Add assert.deepOwnPropertyVal
Add assert.notDeepOwnPropertyVal

Closes #795.

vieiralucas · 2016-09-28T14:09:35Z

So, chai will only throw if own is used alongside nested flags when a property assertion is made.

Which means that there is "nothing wrong" with this:

expect('test').to.have.own.nested.length(4);

I'm ok with that.

@meeber, this is awesome work. I can not say anything but LGTM

lucasfcosta

Hey @meeber, excellent work (as always 😄)
The whole logic and implementation looks complete on every interface and on the core, however I suggested some minor details on comments just to make sure we're being clear and also one more test to assure we're unable to use own and nested together.
Also, what do you think about changing those multiple combined ternary operators into if/else clauses?

Please let me know if you disagree with anything I said.
Thanks for this PR 😄

lucasfcosta · 2016-09-28T19:35:36Z

lib/chai/core/assertions.js

-      , value = isNested
-        ? pathInfo.value
-        : obj[name];
+      , hasProperty = isOwn ? Object.prototype.hasOwnProperty.call(obj, name)


I'm not sure I like all these ternary operators.
What do you guys think about using if and else clauses?
This is totally understandable, but making this behavior more clear might make these conditions look better and more easily understandable.

But all these nested ternaries like you :(

I'll change :(

Hahahahaha, I see no problem in keeping them there, it's just a matter of personal taste, would you like to keep them?
If you do, please don't be afraid to say it, there's no problem if you prefer that way 😄

No, I'd rather be sad :(

Plus I checked @keithamus's eslint config and he doesn't like nested ternaries either :(

lucasfcosta · 2016-09-28T19:39:21Z

lib/chai/interface/assert.js

-   * Asserts that `object` has a property named by `property` with value given
-   * by `value`. `property` can use dot- and bracket-notation for nested
-   * reference. Uses a strict equality check (===).
+   * Asserts that `object` has a direct property named by `property`.


I think that only saying "direct property" isn't enough to make this explanation clear, maybe we could add (non-inherited) after the word direct just to make sure we're being clear.

I don't think this is needed in the other methods where we mention inherited properties, since that contrast already explains everything by itself.

Agreed. I actually had something to the effect of "but not inherited" in a previous iteration, but ended up removing it because the way I had it worded made it sound like the assertion would fail if the object had an inherited property with the given name, whereas it's allowed to have an inherited property with the given name as long as it also has a direct property with the same name. I should be able to reword it to not sound ambiguous though.

lucasfcosta · 2016-09-28T19:40:52Z

lib/chai/interface/assert.js

+  /**
+   * ### .notOwnProperty(object, property, [message])
+   *
+   * Asserts that `object` does _not_ have a direct property named by `property`.


Same here as on L1183.

lucasfcosta · 2016-09-28T19:41:08Z

lib/chai/interface/assert.js

+  /**
+   * ### .ownPropertyVal(object, property, value, [message])
+   *
+   * Asserts that `object` has a direct property named by `property` and a value


Same here as on L1183.

lucasfcosta · 2016-09-28T19:41:18Z

lib/chai/interface/assert.js

-   * Asserts that `object` does _not_ have a property named by `property` with
-   * value given by `value`. `property` can use dot- and bracket-notation for
-   * nested reference. Uses a strict equality check (===).
+   * Asserts that `object` does _not_ have a direct property named by `property`


Same here as on L1183.

lucasfcosta · 2016-09-28T19:41:27Z

lib/chai/interface/assert.js

-   * Asserts that `object` has a property named by `property` with a value given
-   * by `value`. `property` can use dot- and bracket-notation for nested
-   * reference. Uses a deep equality check.
+   * Asserts that `object` has a direct property named by `property` and a value


Same here as on L1183.

lucasfcosta · 2016-09-28T19:41:36Z

lib/chai/interface/assert.js

-   * Asserts that `object` does _not_ have a property named by `property` with
-   * value given by `value`. `property` can use dot- and bracket-notation for
-   * nested reference. Uses a deep equality check.
+   * Asserts that `object` does _not_ have a direct property named by `property`


Same here as on L1183.

lucasfcosta · 2016-09-28T19:46:53Z

test/expect.js

-      expect({ 'foo.bar': 'baz' })
-        .to.have.nested.property('foo.bar');
-    }, "expected { 'foo.bar': 'baz' } to have a nested property 'foo.bar'");
+    }, "expected { foo: { bar: 'baz' } } to have property 'foo.bar'");
  });

  it('property(name, val)', function(){


IMO it would be good to have tests to see if we're really unable to use both own and nested together.

lucasfcosta · 2016-09-28T19:47:24Z

test/should.js

-    err(function(){
-      ({ 'foo.bar': 'baz' }).should.have.nested.property('foo.bar');
-    }, "expected { 'foo.bar': 'baz' } to have a nested property 'foo.bar'");
+    }, "expected 'asd' to have property 'foo'");
  });

  it('property(name, val)', function(){


IMO it would be good to have tests to see if we're really unable to use both own and nested together.

- Add `own` flag - Make `ownProperty` and `haveOwnProperty` aliases of `own.property` - Add `deep` support to `own.property` - Add `assert.deepOwnPropertyVal` - Add `assert.notDeepOwnPropertyVal`

meeber · 2016-09-28T21:32:08Z

Pushed new version with better comments, more tests, and fewer glorious ternaries.

lucasfcosta · 2016-09-28T21:40:18Z

LGTM!
Awesome work @meeber, I'll leave this here for you to merge if you want to use the ternaries we had before 😄
If you are happy with the current version of this please feel free to hit the merge button!

meeber · 2016-09-28T21:51:02Z

@lucasfcosta Thanks for reviewing! Good catches :D

I'll leave this open another day for @keithamus and/or @vieiralucas to look at since I made some changes.

I like it fine without the fabulous ternaries, just giving you a hard time :D

Standardize failed property assertion messages

1f36393

meeber changed the title ~~Refactor own property~~ Refactor ownProperty Sep 28, 2016

lucasfcosta requested changes Sep 28, 2016

View reviewed changes

Refactor ownProperty

edfe064

- Add `own` flag - Make `ownProperty` and `haveOwnProperty` aliases of `own.property` - Add `deep` support to `own.property` - Add `assert.deepOwnPropertyVal` - Add `assert.notDeepOwnPropertyVal`

meeber force-pushed the refactor-own-property branch from 97f8725 to edfe064 Compare September 28, 2016 21:24

lucasfcosta approved these changes Sep 28, 2016

View reviewed changes

meeber merged commit 5dd96a0 into chaijs:master Sep 29, 2016

meeber deleted the refactor-own-property branch September 29, 2016 20:12

lucasfcosta mentioned this pull request Oct 2, 2016

4.x.x Complete Migration Guide #781

Closed

astorije mentioned this pull request Dec 18, 2016

Update for Chai v4.0 astorije/chai-immutable#69

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `ownProperty` #810

Refactor `ownProperty` #810

meeber commented Sep 28, 2016

vieiralucas commented Sep 28, 2016

lucasfcosta left a comment

lucasfcosta Sep 28, 2016

meeber Sep 28, 2016

lucasfcosta Sep 28, 2016

meeber Sep 28, 2016

lucasfcosta Sep 28, 2016 •

edited

Loading

meeber Sep 28, 2016

lucasfcosta Sep 28, 2016

lucasfcosta Sep 28, 2016

lucasfcosta Sep 28, 2016

lucasfcosta Sep 28, 2016

lucasfcosta Sep 28, 2016

lucasfcosta Sep 28, 2016

meeber Sep 28, 2016

lucasfcosta Sep 28, 2016

meeber commented Sep 28, 2016

lucasfcosta commented Sep 28, 2016

meeber commented Sep 28, 2016

Refactor ownProperty #810

Refactor ownProperty #810

Conversation

meeber commented Sep 28, 2016

vieiralucas commented Sep 28, 2016

lucasfcosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meeber commented Sep 28, 2016

lucasfcosta commented Sep 28, 2016

meeber commented Sep 28, 2016

Refactor `ownProperty` #810

Refactor `ownProperty` #810

lucasfcosta Sep 28, 2016 •

edited

Loading