FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: argument types for assert methods by ameliavoncat · Pull Request #11548 · nodejs/node · GitHub

/ node Public

doc: argument types for assert methods - #11548

Closed
ameliavoncat wants to merge 1 commit into
nodejs:masterfrom
ameliavoncat:assert-data-types
ConversationCommits (1)ChecksFiles changed
Closed

doc: argument types for assert methods#11548
ameliavoncat wants to merge 1 commit into
nodejs:masterfrom
ameliavoncat:assert-data-types

Conversation

ameliavoncat commented Feb 25, 2017

Copy link
Copy Markdown
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 added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. labels Feb 25, 2017
TimothyGu reviewed Feb 25, 2017
View reviewed changes
Comment thread doc/api/assert.md Outdated

TimothyGu Feb 25, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

TimothyGu suggested changes Feb 25, 2017
View reviewed changes
Comment thread doc/api/assert.md Outdated

TimothyGu Feb 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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().

Comment thread doc/api/assert.md Outdated

TimothyGu Feb 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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).

Comment thread doc/api/assert.md Outdated

TimothyGu Feb 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

ameliavoncat commented Feb 25, 2017

Copy link
Copy Markdown
Contributor Author

Thanks for the review - changes made.

lpinca reviewed Feb 25, 2017
View reviewed changes
Comment thread doc/api/assert.md Outdated

lpinca Feb 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ameliavoncat Feb 25, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done.

joyeecheung reviewed Feb 25, 2017
View reviewed changes
Comment thread doc/api/assert.md Outdated

joyeecheung Feb 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread doc/api/assert.md Outdated

joyeecheung Feb 25, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should be any, like assert.ok

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

PR-URL: #11548
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell commented Feb 27, 2017

Copy link
Copy Markdown
Member

Landed in 76e3e49

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

PR-URL: nodejs#11548
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas mentioned this pull request Feb 28, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 7, 2017
doc: argument types for assert methods
d63a712 
Refs: #9399

PR-URL: #11548
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
doc: argument types for assert methods
4403975 
Refs: #9399

PR-URL: #11548
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
doc: argument types for assert methods
bfa3989 
Refs: #9399

PR-URL: #11548
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
doc: argument types for assert methods
09c9105 
Refs: #9399

PR-URL: #11548
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins mentioned this pull request Mar 9, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

joyeecheung joyeecheung left review comments
jasnell jasnell approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
+2 more reviewers
TimothyGu TimothyGu approved these changes
hiroppy hiroppy approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL