[WIP] Verify types on within and related assertions #692

vieiralucas · 2016-04-30T21:10:35Z

Hello there, this will be my proposal solution for #691

So why this is a WIP...
I started coding this from the branch master.
When I came here to make the PR, I realized that the 4.x.x branch was 166 commits behind master and that my PR could not be merged.
Well, no problem. I checkout to 4.x.x and made some cherry-picks (actually ctrl-c ctrl-v 😄)
BUT, I saw that some features that exists on master did not exist here on 4.x.x

The features which are missing on 4.x.x that affect this PR:

assert.atLeast
assert.atMost
increase|decrease...by()

What should I do about it?

Things that I'm not sure how to do

Documentation updated to indicate type requirements

meeber · 2016-04-30T23:08:21Z

@vieiralucas this is some good stuff. Thanks for working on this! :D

Documentation is generated from the function comments. Elsewhere in the documentation, the convention used for this type of situation is: "asserts that the string target is...". So we'll want to do something similar in the comments for the functions modified in the PR.

My suspicion is that we'll want your PR to include all the changes (including the atLeast and atMost parts that haven't been merged from master yet), even though we won't be able to merge it until master is merged with 4.x.x. But I could be wrong so let's wait for @keithamus to offer guidance.

Also, just to put this on everyone's radar: there's currently no within on the assert interface.

lucasfcosta · 2016-05-01T16:23:59Z

lib/chai/core/assertions.js

@@ -504,14 +504,15 @@ module.exports = function (chai, _) {
  /**
   * ### .above(value)
   *
-   * Asserts that the target is greater than `value`.
+   * Asserts that the number target is greater than `value`.


Just being nitpicky, but this should be:

Asserts that the target number is greater than value

lucasfcosta · 2016-05-01T16:32:14Z

Thanks for the PR @vieiralucas, great to see you here!

This is looking really good, especially when it comes to the tests, you did a great work covering your changes! I just think we may change some of the texts to make them clearer.

@meeber Maybe we could create an issue for that, adding within to the assert interface won't be much of a problem, but it would be good to have it registered somewhere so we won't end up forgetting.

I think it won't hurt to include the changes on master here (atLeast and atMost) since all we've gotta do when merging will be resolve the conflict (which will be just about in which line is which thing) on these parts of code. But that's just my opinion, I'm sure @keithamus will know better what to do 😄

meeber · 2016-05-01T16:43:00Z

@lucasfcosta The only issue is that the existing documentation says stuff like "string target" and "function target" all over the place, so while I also prefer "target number" over "number target", it does break the established style. Although we could just change all the existing ones :D

Kinda the same thing with the argument error message phrasing. He just used the phrasing style that already exists from the closeTo assertion. If we also want to change all these + the existing one, then I think merely adding a period would probably do the trick: the argument to .above must be a number.

Other alternatives:
the argument provided to .above must be a number
.above only accepts a numeric argument
The .above assertion only accepts a numeric argument
Invalid argument passed to .above: Number required
WARNING: A NON-NUMERIC ARGUMENT HAS BEEN DETECTED IN THE .above ASSERTION. LAUNCHING MISSILES IN 5... 4.... 3....

vieiralucas · 2016-05-01T16:45:34Z

I would definitely go with the missile one

vieiralucas · 2016-05-04T02:28:43Z

So, should I try to merge the missing features and implement the type check?
I could also implement within on the assert interface.

keithamus · 2016-05-04T08:53:50Z

I think it's probably time to merge 4.x.x into master and cut 4.0.0 as a release (including this PR). What do you think @lucasfcosta @meeber?

meeber · 2016-05-04T10:58:32Z

@keithamus That sounds good. We'll need to get the check-error module added as well. Also it'd be good to have the assertion-error module released as a new version and updated here; it contains an important bug fix.

Do you want @vieiralucas to update this PR with the fixes that are currently master-only (related to the atLeast, atLmost, and increase/decrease assertions, which haven't been merged into 4.x.x yet)?

@vieiralucas Please hold off on the within thing; let's tackle that (+ any other missing assertions on the assert interface) on a separate PR post-4.0.0 release.

lucasfcosta · 2016-05-04T11:55:41Z

@keithamus I think it would be great! Many people have asked for features on the 4.x.x branch until now, maybe it's time to make their wishes come true 😄

@meeber Do you think we should create an issue for that? Maybe it would be good, just to keep this on our sight so we won't forget.

Let me know if you guys need any help for this merge, I think it will be a pretty big one.
My new notebook may arrive before friday, I can help you with some code as soon as I receive it since my old notebook isn't with me anymore 😢

meeber · 2016-05-04T12:01:05Z

@lucasfcosta Definitely. I've been wanting to sit down and go through and identify all the assertions that are missing from the assert interface and create an issue that covers all of them but haven't had a chance yet.

keithamus · 2016-05-04T12:04:21Z

Let's get whats on this PR merged into 4.x.x. We can raise another PR later for the missing assert methods and make sure they're all behaving the same. The sooner we can get this branch merged, the sooner we can rebase 4.x.x, the sooner we can release. Everyone happy with this?

vieiralucas · 2016-05-04T12:09:16Z

I couldn't be happier 😄

meeber · 2016-05-04T12:12:52Z

@keithamus I just want to make sure we're on the same page here. There are two separate issues:

There are a few numeric assertions currently on master but not on 4.x.x: atLeast, atMost, and increase/decrease. @vieiralucas originally wrote this PR against the master branch, providing type checking on those assertions. But when he went to open a PR, he saw that they weren't in 4.x.x and thus removed them so there weren't any conflicts. Do you want him to resubmit his original PR that covers those assertions before we merge? He just posted in this thread so it seems he's active.
It was also discovered that the within assertion doesn't exist on the assert interface. @lucasfcosta and I were discussing opening a separate issue for that + any other missing assertions. Agreed that this should be handled after 4.x.x is release.

keithamus · 2016-05-04T12:17:13Z

@meeber yup all on the same page. Here's my 2¢:

Merge this PR, get 4.x.x rebased and merge in. Then we can tackle atLeast, atMost, increase and decrease in a separate PR. This way, at least IMO, we're not wasting any work and only rolling forward.
Once the above is done, we tackle assert.within in a new separate PR. This will go against the new master, which will be effectively 4.x.x.

meeber · 2016-05-04T12:18:08Z

@keithamus Sounds good! :D

keithamus · 2016-05-04T12:19:36Z

lib/chai/core/assertions.js

@@ -553,14 +566,15 @@ module.exports = function (chai, _) {
  /**
   * ### .least(value)
   *
-   * Asserts that the target is greater than or equal to `value`.
+   * Asserts that the number target is greater than or equal to `value`.


"target number" here would be better than "number target"

keithamus · 2016-05-04T12:21:31Z

@vieiralucas apart from the tiny grammatical errors which would be great to see fixed, I'm happy with this PR and would be happy to merge when those errors are fixed 👍

vieiralucas · 2016-05-04T12:30:52Z

@keithamus Alright, but have you seen that existing documentation is like "string target" and "function target"?. I wanted to maintain consistency. It's up to you guys, I can, with no problem, fix that.

keithamus · 2016-05-04T12:32:12Z

@vieiralucas hmm, I didn't catch that. Lets keep it consistent for now then, and we can take a look at documentation later down the line.

In which case this LGTM, @meeber or @lucasfcosta feel free to merge if it LGTY 😉

meeber · 2016-05-04T12:33:25Z

LGTM!

Verify types on within and related assertions

78c5763

Documentation update to indicate type requirements

de86590

lucasfcosta reviewed May 1, 2016
View reviewed changes

keithamus reviewed May 4, 2016
View reviewed changes

meeber merged commit 4908550 into chaijs:4.x.x May 4, 2016

vieiralucas deleted the 4.x.x branch May 4, 2016 12:35

meeber mentioned this pull request Sep 10, 2016

Merge rebased 4.x.x branch into master #783

Merged

vieiralucas mentioned this pull request Sep 24, 2016

Refactor .ownProperty to leverage .property #795

Closed

lucasfcosta mentioned this pull request Oct 2, 2016

4.x.x Complete Migration Guide #781

Closed

meeber mentioned this pull request May 31, 2017

Error or not updated docs on assert.isAbove and isBelow #980

Closed

v1adko mentioned this pull request Jun 6, 2017

Allow dates for isBelow and isAbove assertions #990

Merged

vieiralucas mentioned this pull request Jul 27, 2017

lt, lte, gt & gte no more work with strings or mongodb ObjectIds #1011

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Verify types on within and related assertions #692

[WIP] Verify types on within and related assertions #692

vieiralucas commented Apr 30, 2016 •

edited

meeber commented Apr 30, 2016

lucasfcosta May 1, 2016

lucasfcosta commented May 1, 2016

meeber commented May 1, 2016

vieiralucas commented May 1, 2016

vieiralucas commented May 4, 2016

keithamus commented May 4, 2016

meeber commented May 4, 2016

lucasfcosta commented May 4, 2016

meeber commented May 4, 2016

keithamus commented May 4, 2016

vieiralucas commented May 4, 2016

meeber commented May 4, 2016

keithamus commented May 4, 2016

meeber commented May 4, 2016

keithamus May 4, 2016

keithamus commented May 4, 2016

vieiralucas commented May 4, 2016

keithamus commented May 4, 2016

meeber commented May 4, 2016

[WIP] Verify types on within and related assertions #692

[WIP] Verify types on within and related assertions #692

Conversation

vieiralucas commented Apr 30, 2016 • edited

meeber commented Apr 30, 2016

lucasfcosta May 1, 2016

Choose a reason for hiding this comment

lucasfcosta commented May 1, 2016

meeber commented May 1, 2016

vieiralucas commented May 1, 2016

vieiralucas commented May 4, 2016

keithamus commented May 4, 2016

meeber commented May 4, 2016

lucasfcosta commented May 4, 2016

meeber commented May 4, 2016

keithamus commented May 4, 2016

vieiralucas commented May 4, 2016

meeber commented May 4, 2016

keithamus commented May 4, 2016

meeber commented May 4, 2016

keithamus May 4, 2016

Choose a reason for hiding this comment

keithamus commented May 4, 2016

vieiralucas commented May 4, 2016

keithamus commented May 4, 2016

meeber commented May 4, 2016

vieiralucas commented Apr 30, 2016 •

edited