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

.lengthOf for Maps and Sets #1131

Merged
merged 2 commits into from
Feb 11, 2018
Merged

Conversation

asbish
Copy link
Contributor

@asbish asbish commented Jan 26, 2018

As described in #1110, this PR adds support for Map and Set to .lengthOf and chains such as .above, .below, .least, .most and .within.
I have not added alias 'size' to .lengthOf this time so as not to fix some exist test cases (Levenshtein distance between 'size' and 'pizza' used in the test cases is close). Will you give me some feedback on the above?
Feel free to point out possible mistakes. Thanks!

@asbish asbish requested a review from a team as a code owner January 26, 2018 12:43
@codecov
Copy link

codecov bot commented Jan 26, 2018

Codecov Report

Merging #1131 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1131     +/-   ##
=========================================
+ Coverage   93.69%   93.79%   +0.1%     
=========================================
  Files          32       32             
  Lines        1649     1676     +27     
  Branches      396      404      +8     
=========================================
+ Hits         1545     1572     +27     
  Misses        104      104
Impacted Files Coverage Δ
lib/chai/interface/assert.js 89.92% <ø> (ø) ⬆️
lib/chai/core/assertions.js 99.39% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 914665b...7ec14ad. Read the comment docs.

@meeber
Copy link
Contributor

meeber commented Jan 26, 2018

@asbish Oh nooo we've been foiled again by pizza. Thanks so much for working on this. It's going to be awhile until I can review; working crazy hours this week and then going on vacay next week. I'll try to carve out some time.

Copy link
Contributor

@meeber meeber left a comment

Choose a reason for hiding this comment

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

@asbish The code and tests look excellent to me. I appreciate your thoroughness.

I added a couple of inline comments regarding the docs for .lengthOf.

Also, we should update the docs for the other assertions. For example, lines 1105-1106 could be updated to say "Add .lengthOf earlier in the chain to assert that the target's length or size is greater than the given number n." (It's probably overkill to say "the value of" and "property". It's probably also overkill to explicitly state that size is for Map and Set. But I don't feel strongly about either of those statements.)

Finally, we should update the docs for assert.lengthOf.

@@ -2013,6 +2048,11 @@ module.exports = function (chai, _) {
* expect([1, 2, 3]).to.have.lengthOf(3);
* expect('foo').to.have.lengthOf(3);
*
* When the target is a map or set, asserts that the target's `size` property.
Copy link
Contributor

Choose a reason for hiding this comment

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

This reads awkwardly to me. Instead of having this separate line, I suggest updating lines 2045-2046 to read: "Asserts that the target's length or size is equal to the given number n."

And then the new examples could go right beneath the "foo" example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

* When the target is a map or set, asserts that the target's `size` property.
*
* expect(new Set([1, 2])).to.hove.lengthOf(2);
* expect(new Map([['a', 1], ['b', 2]])).to.hove.lengthOf(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a typo in these examples: "hove" instead of "have".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@asbish
Copy link
Contributor Author

asbish commented Feb 11, 2018

@meeber Thank you so mush for your review!
I have fixed, including the commit of the indentation in assertions.js at 2124
Following your suggestion have made the documentation much clearer and easier to read:smile:

@meeber
Copy link
Contributor

meeber commented Feb 11, 2018

LGTM! Looking for second review from @chaijs/chai

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.

3 participants