Conversation
doc/api/assert.markdown
Outdated
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 |
doc/api/assert.markdown
Outdated
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 |
doc/api/assert.markdown
Outdated
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.
doc/api/assert.markdown
Outdated
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 |
doc/api/assert.markdown
Outdated
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! |
doc/api/assert.markdown
Outdated
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" notifications@github.com 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. |
doc/api/assert.markdown
Outdated
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 <trev.norris@gmail.com>
|
Landed in 3e648eb |
General improvements to assert.markdown copy including new and improved examples PR-URL: nodejs#4360 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
General improvements to assert.markdown copy including new and improved examples PR-URL: #4360 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
General improvements to assert.markdown copy including new and improved examples PR-URL: #4360 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
General improvements to assert.markdown copy including new and improved examples PR-URL: nodejs#4360 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
9 participants
General improvements to assert.markdown, including examples