doc: improve assert.markdown copy #4360
Conversation
| `require('assert')`. However, it is recommended that a userland assertion | ||
| library be used instead. | ||
| The `assert` module was originally written and included in Node.js in order | ||
| to facilitate Node.js' own unit tests. While it can be used by JavaScript |
There was a problem hiding this comment.
Node.js' own unit tests -> unit tests for Node.js itself.
There was a problem hiding this comment.
Nit to facilitate -> for use with
|
@Trott ... updated per your feedback. PTAL |
| library be used instead. | ||
| The `assert` module is for use with Node.js' own unit tests. While it can be | ||
| used by JavaScript running within Node.js by using `require('assert')`, it is | ||
| recommended that a third-party ("userland") assertion module be used instead. |
There was a problem hiding this comment.
At a minimum, I would remove by JavaScript running so you just have:
While it can be used within Node.js by using
require('assert'), it is recommended that a....
There was a problem hiding this comment.
third-party may be problematic. If the user writes their own userland assertion module, then it's second-party, right? I may be overthinking this.
|
@Trott ... fixed! |
jasnell
force-pushed
the
doc-assert-improvements
branch
from
07d0e2c
to
a7a8753
Compare
|
@nodejs/ctc @nodejs/collaborators ... can I please get some additional review on this one |
| This module is used so that Node.js can test itself. It can be accessed with | ||
| `require('assert')`. However, it is recommended that a userland assertion | ||
| library be used instead. | ||
| The `assert` module is for use with Node.js' own unit tests. While it can be |
There was a problem hiding this comment.
The tendency to favor Node.js' over Node.js's is so strong that I don't even bother correcting it directly and instead recommend rewording entirely to avoid the possessive. So, for example, something like:
The
assertmodule is for use with unit tests in the source code for Node.js.
| This module is used so that Node.js can test itself. It can be accessed with | ||
| `require('assert')`. However, it is recommended that a userland assertion | ||
| library be used instead. | ||
| The `assert` module is for use with the unit tests in the Node.js source. |
There was a problem hiding this comment.
It is also used (in node's own source and by others), as it is used in C and other languages: to assert invariants in the code. There are lots of unit testing modules around, but I'm not aware of an alternate module for this use. We should avoid characterizing this as a bad unit test library, even if that's one of the things node uses it for.
|
@sam-github ... made another revision to that intro paragraph. PTAL |
| potentially surprising results. For example, this does not throw an | ||
| `AssertionError` because the properties on the [`Error`][] object are | ||
| non-enumerable: | ||
| Only considers enumerable "own" properties are considered. The `deepEqual()` |
There was a problem hiding this comment.
considers is used twice in this sentence and sounds a bit odd
should it be
Only enumerable "own" properties are considered
|
@thealphanerd ... fixed! |
| The API for the `assert` module is [Locked][]. This means that there will be no | ||
| additions or changes to any of the methods implemented and exposed by | ||
| the module. It is recommended that an alternative ("userland" or third-party) | ||
| assertion module be used instead. |
There was a problem hiding this comment.
I assert ;-) that no such modules exist for the purpose for which assert is well-suited, and arguably intended, in-source assertions of invariants.
Can you point to examples of userland modules for this purpose, @jasnell?
Pointing people writing unit tests elsewhere is a good idea, there are lots of unit test frameworks around.
There was a problem hiding this comment.
I cannot. This is just an edit of the existing text. I'd be fine with
removing the recommendation
On Dec 23, 2015 10:03 PM, "Sam Roberts" [email protected] wrote:
In doc/api/assert.markdown
#4360 (comment):@@ -2,42 +2,130 @@
Stability: 3 - Locked-This module is used so that Node.js can test itself. It can be accessed with
-require('assert'). However, it is recommended that a userland assertion
-library be used instead.
+While theassertmodule is generally intended for internal use by Node.js
+itself, it can be used by user code callingrequire('assert').
+
+The API for theassertmodule is [Locked][]. This means that there will be no
+additions or changes to any of the methods implemented and exposed by
+the module. It is recommended that an alternative ("userland" or third-party)
+assertion module be used instead.I assert ;-) that no such modules exist for the purpose for which assert
is well-suited, and arguably intended, in-source assertions of invariants.Can you point to examples of userland modules for this purpose, @jasnell
https://github.com/jasnell?Pointing people writing unit tests elsewhere is a good idea, there are
lots of unit test frameworks around.—
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/4360/files#r48397585.
There was a problem hiding this comment.
Nit: no scare quotes around userland
|
@sam-github @Trott ... updated and removed that recommendation. |
| `assert.equal(!!value, true, message)`. | ||
|
|
||
| If `value` is not truthy, an `AssertionError` is thrown with a `message` | ||
| property set equal to the value of the `message` parameter. If the `message` | ||
| parameter is undefined, a default error message is assigned. |
There was a problem hiding this comment.
Let's see if u I remember how to do this. You mean: undefined
EDIT: auto correct fix.
General improvements to assert.markdown copy including new and improved examples
jasnell
force-pushed
the
doc-assert-improvements
branch
from
8fab051
to
73007e9
Compare
|
@thefourtheye @trevnorris ... fixed, commits squashed. PTAL |
|
LGTM |
General improvements to assert.markdown copy including new and improved examples PR-URL: #4360 Reviewed-By: Trevor Norris <[email protected]>
|
Landed in 3e648eb |
General improvements to assert.markdown copy including new and improved examples PR-URL: nodejs#4360 Reviewed-By: Trevor Norris <[email protected]>
General improvements to assert.markdown copy including new and improved examples PR-URL: #4360 Reviewed-By: Trevor Norris <[email protected]>
General improvements to assert.markdown copy including new and improved examples PR-URL: #4360 Reviewed-By: Trevor Norris <[email protected]>
General improvements to assert.markdown copy including new and improved examples PR-URL: nodejs#4360 Reviewed-By: Trevor Norris <[email protected]>
General improvements to assert.markdown, including examples