refactor: use constants for event names instead of string literals #3655
Conversation
boneskull
added
semver-patch
boneskull
force-pushed
the
boneskull/refactor/event-constants
branch
from
f55b965 to
246a4de
Compare
|
Instead of the wiki, I've added a tutorial in the API documentation section. Once this is merged & released we can redirect users to it. |
boneskull
force-pushed
the
boneskull/refactor/event-constants
branch
from
11a92c5 to
c0d1d92
Compare
boneskull
force-pushed
the
boneskull/refactor/event-constants
branch
from
6297486 to
5e55bc8
Compare
| [runner]: /api/mocha.runner | ||
| [test]: /api/mocha.test | ||
| [hook]: /api/mocha.hook | ||
| [suite]: /api/mocha.suite |
There was a problem hiding this comment.
Why was the Wiki page poo-poo'd in favor of this?
There was a problem hiding this comment.
I would like to consolidate tutorials on the documentation site. It is painful to have to go to separate sites for information.
I am not thrilled about how the tutorial is buried in the navigation, but #3663 should allow us to move the "Tutorials" nav section to the top.
| @@ -8,6 +8,9 @@ | |||
|
|
|||
| var Base = require('./base'); | |||
There was a problem hiding this comment.
This is a question, not request for change:
As all reporters besides html and json-stream (which can do both) are Node-based, are the non-browser reporters required to retain ES5-compatiblity?
There was a problem hiding this comment.
I think, simply because they're all pulled in to the bundle. Not pulling them in to would break people using certain headless browser setups.
Ideally we shouldn't have to manage the language level on a file-by-file or directory-by-directory basis, and would have a transpilation step. I don't want to have to think about it, and neither should our contributors.
There was a problem hiding this comment.
(yes, to pile on w/ the confusion, the node.js-based reporters can and do run in a browser on the command-line)
There was a problem hiding this comment.
Wish I could say I understand what you just wrote, but I don't. Many are terminal-based (e.g., Nyan) and should fail if attempted to run in browser. I was about to add more error checking which would cause Error to be thrown if terminal cursor positioning was unsupported in a new PR. You saying that won't work due to browser?
There was a problem hiding this comment.
See what mochify does. Essentially it takes Mocha and pipes it thru a headless browser, but it looks like it's running as it would with Node.js. That means you can use console-based reporters with it.
It's unclear to me if what you're doing would cause a problem. My suggestion is to download Mochify and npm link it against your mocha working copy & see what happens.
| @@ -2,14 +2,22 @@ | |||
|
|
|||
| /** | |||
| * Provides a factory function for a {@link StatsCollector} object. | |||
| * @private | |||
| * @module | |||
There was a problem hiding this comment.
* @module StatsCollectorThere was a problem hiding this comment.
...StatsCollector is a typedef. we don't want to conflicting names. it seems like it should refer to what you'd actually call require with: require('mocha/lib/stats-collector'), so stats-collector.
it defaults to lib/stats-collector, which is fine for now, I guess.
There was a problem hiding this comment.
I mean, the docstrings need cleanup. I am not convinced we're all on the same page about what belongs in the API docs and how best to present it. ...or even how to come to that decision. Maybe someone can just propose something and throw it in a PR and we can go from there?
boneskull
force-pushed
the
boneskull/refactor/event-constants
branch
from
d3571ed to
9642e60
Compare
|
|
||
| [Base] is an "abstract" class. It contains console-specific settings and utilities, commonly used by built-in reporters. Browsing the built-in reporter implementations, you may often see static properties of [Base] referenced. | ||
|
|
||
| It's often useful--but not necessary--for a custom reporter to extend [Base]. |
There was a problem hiding this comment.
Add spaces on both sides of dbl-hyphens so spelling can be checked more easily
There was a problem hiding this comment.
I've never heard of that issue before, can you explain more?
I can say with certainty that I don't love double-hyphens! Maybe there's a smart hyphen transformation...
There was a problem hiding this comment.
Would prefer:
It's often useful--but not necessary--for
changed to:
It's often useful -- but not necessary -- for
or
It's often useful (but not necessary) for
boneskull
force-pushed
the
boneskull/refactor/event-constants
branch
from
9642e60 to
4b90caa
Compare
boneskull
force-pushed
the
boneskull/refactor/event-constants
branch
from
0c76c95 to
50bfcd7
Compare
- also a few other string literals got replaced, but not error messages nor codes
- supporting refactors:
- move the suite "GC" function into the `Suite` prototype
- move stats collector init to `Mocha#run` due to circular reference with `Runner`
- rename a couple fixture files lacking proper extension
- rename variable in integration test helpers
- add `utils.createMap()` and `utils.defineConstants()`
- add `test/integration/fixtures` to `.eslintignore` so no fixture linting occurs whatsoever
- consolidated use of `object.assign` polyfill into `utils` module
- some docstring fixes/consistency
- adds `EVENT_DELAY_END` emitted from `Runner` for reporter use
- specifically did NOT refactor event names from Node.js incl. `error` and `uncaughtException`
- add custom reporter tutorial to API documentation
- automatically injects reporter example into tutorial via `markdown-magic`
- add `docs.preprocess.api` script
- add JSDoc configuration to support tutorials
- sort JSDoc config object because fussy
- fix broken custom assertion
boneskull
force-pushed
the
boneskull/refactor/event-constants
branch
from
bd114f0 to
51ec4af
Compare
|
squashed, going to merge it |
SuiteprototypeMocha#rundue to circular reference withRunnererroranduncaughtExceptionDid not rename any events.