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

doc: argument types for assert methods #11548

Closed
wants to merge 1 commit into from
Closed

doc: argument types for assert methods #11548

wants to merge 1 commit into from

Conversation

ameliavoncat
Copy link
Contributor

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

documentation

Description of changes

Added argument data types to the docs for the assert module.

Issue

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. labels Feb 25, 2017
TimothyGu
TimothyGu reviewed Feb 25, 2017
View reviewed changes
doc/api/assert.md Outdated
@@ -41,6 +43,9 @@ changes:
pr-url: https://github.com/nodejs/node/pull/5910
description: Handle non-`Uint8Array` typed arrays correctly.
-->
* `actual` {Object|Primitive}
Copy link
Member

@TimothyGu TimothyGu Feb 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

There is already usage of {any} which seems to fit this situation better. Ditto below.

TimothyGu
TimothyGu suggested changes Feb 25, 2017
View reviewed changes
doc/api/assert.md Outdated
@@ -143,6 +151,9 @@ changes:
pr-url: https://github.com/nodejs/node/pull/3276
description: The `error` parameter can now be an arrow function.
-->
* `block` {Function}
* `error` {Error|RegExp|Function}
Copy link
Member

Choose a reason for hiding this comment

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

The documentation doesn't mention how anything other than a RegExp or a Function (either a constructor or a validation function) would work. I.e., I don't think it can be an Error object, only an Error constructor. Ditto for .throws().

doc/api/assert.md Outdated
@@ -9,6 +9,8 @@ test invariants.
<!-- YAML
added: v0.5.9
-->
* `value` {Boolean|Number}
Copy link
Member

Choose a reason for hiding this comment

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

Well, technically any value (not just booleans and numbers) would work. I'd be fine with either any or Boolean. since the concepts of truthy-ness and falsy-ness are defined by Boolean(val).

doc/api/assert.md Outdated
@@ -243,6 +263,7 @@ assert.fail(1, 2, 'whoops', '>');
<!-- YAML
added: v0.1.97
-->
* `value` {Number|String|Error}
Copy link
Member

Choose a reason for hiding this comment

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

Any type would work with this function. The examples below are just that -- examples.

@ameliavoncat ameliavoncat force-pushed the assert-data-types branch 2 times, most recently from 73fbc20 to 9b5d9e8 Compare February 25, 2017 08:09
@ameliavoncat
Copy link
Contributor Author

Thanks for the review - changes made.

lpinca
lpinca reviewed Feb 25, 2017
View reviewed changes
doc/api/assert.md Outdated
* `actual` {any}
* `expected` {any}
* `message` {String}

Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you please remove these spurious/extra new lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

joyeecheung
joyeecheung reviewed Feb 25, 2017
View reviewed changes
doc/api/assert.md Outdated
@@ -379,6 +410,8 @@ If the values are strictly equal, an `AssertionError` is thrown with a
<!-- YAML
added: v0.1.21
-->
* `value` {Boolean}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is actually any given the description below...

doc/api/assert.md Outdated
@@ -9,6 +9,8 @@ test invariants.
<!-- YAML
added: v0.5.9
-->
* `value` {Boolean}
Copy link
Member

Choose a reason for hiding this comment

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

Should be any, like assert.ok

jasnell pushed a commit that referenced this pull request Feb 27, 2017
@ameliavoncat @jasnell
doc: argument types for assert methods
76a3e49 
Refs: #9399

PR-URL: #11548
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member

jasnell commented Feb 27, 2017

Landed in 76e3e49

@jasnell jasnell closed this Feb 27, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 28, 2017
@ameliavoncat @italoacasas
doc: argument types for assert methods
128f812 
Refs: nodejs#9399

PR-URL: nodejs#11548
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@italoacasas italoacasas mentioned this pull request Feb 28, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@ameliavoncat @jasnell
doc: argument types for assert methods
d63a712 
Refs: #9399

PR-URL: #11548
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@ameliavoncat @jasnell
doc: argument types for assert methods
4403975 
Refs: #9399

PR-URL: #11548
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@ameliavoncat @MylesBorins
doc: argument types for assert methods
bfa3989 
Refs: #9399

PR-URL: #11548
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@ameliavoncat @MylesBorins
doc: argument types for assert methods
09c9105 
Refs: #9399

PR-URL: #11548
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@cjihrig cjihrig cjihrig approved these changes

@hiroppy hiroppy hiroppy approved these changes

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ameliavoncat @jasnell @lpinca @TimothyGu @cjihrig @hiroppy @joyeecheung @nodejs-github-bot