add exists alias #1227
add exists alias #1227
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1227 +/- ##
==========================================
+ Coverage 94.51% 94.51% +<.01%
==========================================
Files 32 32
Lines 1676 1678 +2
Branches 404 404
==========================================
+ Hits 1584 1586 +2
Misses 92 92
Continue to review full report at Codecov.
|
blake-regalia
force-pushed
the
add-exists-alias
branch
from
6118a07 to
a4e382a
Compare
There was a problem hiding this comment.
@blake-regalia Thanks for submitting this. Let's add a couple of tests to the expect and should interfaces. Duplicates of these two and these two using exists instead of exist should suffice. It's okay if the grammar is wrong.
lib/chai/core/assertions.js
Outdated
| @@ -814,23 +814,32 @@ module.exports = function (chai, _) { | |||
| * expect(undefined).to.be.undefined; // Recommended | |||
| * expect(undefined).to.not.exist; // Not recommended | |||
| * | |||
| * Use after `.property` to assert that a value is not `null` nor `undefined`. | |||
| * expect({a: 1}}).to.have.property('a').that.exists; | |||
There was a problem hiding this comment.
I'm not sure I like this example. For the most part, we discourage the use of .exists in favor of a precise assertion whenever reasonable. For example:
expect({a: 1}).to.have.property('a', 1); // Recommended
expect({a: 1}).to.have.property('a').that.equals(1); // Recommended
expect({a: 1}).to.have.property('a').that.exists; // Not recommendedThere was a problem hiding this comment.
Agreed. I'll remove this part. In fact the use case that motivated this PR is for an interface test suite that requires an implementation to have some property that is set to some value which is not null nor undefined, a less common situation where the actual value (as well as the type) of the property is not known/important.
test/should.js
Outdated
| should.not.exist(bar); | ||
| should.not.exists(bar); |
There was a problem hiding this comment.
I'm sorry, it looks like I gave bad guidance here. I see now that the exist assertion is exposed in a different way for the should interface. I don't think there's any need to add a plural version for the should interface, so we can remove these tests.
There was a problem hiding this comment.
@blake-regalia Do you have a few min to remove these .should tests? Apologies for the inconvenience.
There was a problem hiding this comment.
No problem @meeber ! Thanks for following up with this.
|
This PR looks good, but I'm not sure if we should merge this since we will start merging |
|
@vieiralucas I'm happy to get this merged in as it's an improvement to the current codebase that has 0 cost. |
|
Yes, I got it wrong about |
Closes #1225 .