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

Refactor .ownProperty to leverage .property #795

Closed
meeber opened this issue Sep 10, 2016 · 3 comments
Closed

Refactor .ownProperty to leverage .property #795

meeber opened this issue Sep 10, 2016 · 3 comments

Comments

@meeber
Copy link
Contributor

meeber commented Sep 10, 2016

There's a divergence between the behavior of the .property and .ownProperty assertions due to refactoring of .property in the master branch and .ownProperty in the 4.x.x branch.

I think the thing to do here is:

  • Create a new own flag.
  • Update .property to restrict checks to own properties when the own flag is set.
  • Change .ownProperty and .haveOwnProperty into simple assertions that set the own flag and then call .property to do the rest.

That way, the code paths for .property and .ownProperty are unified and will never diverge again. It also enables chaining with .deep. For example:

expect(obj).to.have.deep.own.property('kittens', {quantity: 3});

The only breaking change to .ownProperty would be the same breaking change that happened to .property during the refactor in #744.

However, I don't think the nested and own flags mix conceptually, since the intent of nested is to test a property that's multiple levels deep, whereas the purpose of own is to test a property that is set directly on an object as opposed to being inherited. Therefore, combining these two flags would cause .property to throw an error.

I already started playing around with this on the plane. I'll go ahead and finish it if there's consensus.

@lucasfcosta @keithamus?

@lucasfcosta
Copy link
Member

I totally agree! Awesome idea!
I think this is totally correct considering one of the oldest principles of the art of programming, which is: don't repeat yourself.

Doing this would make the code more modular and, as you said, would cause these two assertions to never diverge again since they basically do the same thing but have different assertion subjects.

I just think we need to have a good and self explanatory error message when nested is using alongside own.

Great suggestion 👍

@vieiralucas
Copy link
Member

vieiralucas commented Sep 24, 2016

@lucasfcosta It really seems to me that using nested alongside own does not make sense.
A nested property cannot be an own property of an object, right?

I see two options:

  1. Unset own when setting nested and unset nested when setting own.
    Something similar is done for all and any, I don't know if this is very intuitive, because unlike all and any, own and nested are not opposite of each other.
  2. Throw an error when setting the own flag while nested is already set and vice versa.

If we choose the second option I have a suggestion:

The .own property must not be used alongside .nested property.
The .nested property must not be used alongside .own property.

Yes, this is totally stolen from here

@meeber
Copy link
Contributor Author

meeber commented Sep 24, 2016

I'm strongly in favor of throwing an error when nested and own is combined. I think it's important for testing libraries to be stringent about grammar and not silently fix problems., as it may lead to unexpected behavior for the user.

I should have a chance to work on this tomorrow!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants