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.

Sign up for GitHub

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

Jump to bottom

Verify types on increase | decrease #796

Merged
merged 3 commits into from Sep 12, 2016
Merged

Verify types on increase | decrease #796

merged 3 commits into from Sep 12, 2016

Conversation

vieiralucas
Copy link
Member

Hello there again!

I saw that @meeber did a very good job and now chai 4.x.x is finally merged at master.

So here am I, aiming to solve the rest of #691.
This PR aims to check that the property being tested by increase | decrease is a number.
It also adds tests to assert.atLeast and assert.atMost since after the merge they automatically started to check types.

I don't know if I'm happy with the documentation changes, so please give your opinions on that.

Thank You

@meeber
Copy link
Contributor

meeber commented Sep 10, 2016

Round 2!
Fight!

(LGTM!)

lucasfcosta
lucasfcosta reviewed Sep 11, 2016
View reviewed changes
lib/chai/core/assertions.js
@@ -1961,7 +1961,7 @@ module.exports = function (chai, _) {
/**
* ### .increase(function)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the docs for this could use another name for the argument being passed other than function.

@lucasfcosta
Copy link
Member

Good job @vieiralucas, especially regarding the tests 😄
This is looking good, but perhaps we could just change the name of the argument on the increase and decrease assertions. I think function does not reflect what can be put there, maybe renaming that argument to target would be better.
What do you guys think?

@vieiralucas
Copy link
Member Author

vieiralucas commented Sep 12, 2016 •
edited

Nice one!
I renamed it to target and also added property as the second argument.
Thanks

@lucasfcosta
Copy link
Member

lucasfcosta commented Sep 12, 2016 •
edited

Hi @vieiralucas, thanks for the corrections, good job noticing the property argument too! 😄
I'm sorry to add more comments to this, but I just noticed some things we should fix regarding the docs of these methods, here goes a small list of things to do:

  • Since the property and message arguments are optional they should be represented within brackets, like we do here
  • The docs for the types of the arguments also do not say it's possible to pass a function as target.
  • The docs only talk about a numeric property of an object. They do not make it clear it's also possible to use a function as target

Since this PR tackles all those methods maybe we could do all these fixes right here, what do you think?
If you disagree, I'll be happy to accept merging this since the whole code and tests look really good, however, if we could also fix the docs it would be even better.

Also, sorry for not adding comments to lines and for not seeing this before, unfortunately github only allows me to comment lines which have been changed (or are very very near the changes).

@vieiralucas
Copy link
Member Author

@lucasfcosta You have very good eyes 😄

I definitely agree with the things you have said.
Already made the changes.
Please let me know if I missed something.

Thanks again

@vieiralucas
Improve docs for "increase | decrease"
5f3c4ae 
Indicate that 'property' and 'message' are optional.
Indicate that 'increase' and 'decrease' can receive a function which
returns the property to be checked.
@meeber
Copy link
Contributor

meeber commented Sep 12, 2016

LGTM!

@lucasfcosta
Copy link
Member

LGTM!
Good job, I'm very happy to merge this 😄

@lucasfcosta lucasfcosta merged commit 662b76c into chaijs:master Sep 12, 2016
@vieiralucas vieiralucas deleted the verify-type-round2 branch September 12, 2016 20:31
@vieiralucas
Copy link
Member Author

vieiralucas commented Sep 12, 2016 •
edited

\o/

Does that means that #691 can be closed?

@lucasfcosta @meeber

@lucasfcosta
Copy link
Member

Yes it does! Thanks for noticing 👍

@lucasfcosta lucasfcosta mentioned this pull request Oct 2, 2016
Closed
@vieiralucas vieiralucas mentioned this pull request Jul 27, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vieiralucas @meeber @lucasfcosta