index.js: Configurable reporter options for Formatter#49
Open
wking wants to merge 1 commit intotapjs:mainfrom
Open
index.js: Configurable reporter options for Formatter#49wking wants to merge 1 commit intotapjs:mainfrom
wking wants to merge 1 commit intotapjs:mainfrom
Conversation
06efe96 (Fix xunit reporter, 2015-04-25) added a hard-coded empty object for the reporter options. This commit allows the user to override that with their own config, falling back to an empty object if the given value is falsy (for backwards compat with existing Formatter(...) callers). If Formatter wasn't already taking a runtime-options argument, I would have preferred: function Formatter (type, options) { ... var runner = this.runner = new Runner((options || {}).runner) this.reporter = new reporters[type]( this.runner, (options || {}).reporter || {}, ) Writable.call(this, (options || {}).runner) ... } but we can't add 'runner' namespacing to the existing options argument without breaking backwards compat or adding brittle heuristic translation. So I'm just adding a new option, where the reporter is namespaced to allow for other configurable aspects to also use the new argument (if we grow other configurable aspects in the future).
wking
added a commit
to wking/node-tap
that referenced
this pull request
Jun 8, 2018
This PR depends on tapjs/tap-mocha-reporter#49; review that first. This is inspired by [Mocha's][1]: ``` --reporter-options <k=v,k2=v2,...> ``` Except: * I'm using a JSON value for easier parsing and more explicit typing. This will end up using a few more characters, but the format is more explicit than Mocha's (which only uses string values?). And callers are likely to already be familiar with JSON, so we save the mental overhead of teaching them a new serialization format. * I'm using a 'reporter' namespace. This allows us to add other namespaces in the future to address other configurable aspects of tap-mocha-reporter's Formatter. For example, we could use this same CLI option to configure the runner. Somewhat related to this, if we have plans for allowing multiple reporters [2], we may want to namespace *those* now. For example: ``` --reporter-options {"reporter": {"progress": {"open": "(", "close": ")"}}} ``` to avoid the issues Mocha bumped into [3] when trying to add multi-reporter support. [1]: https://mochajs.org/#usage [2]: tapjs#335 [3]: mochajs/mocha#2184 (comment)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
06efe96 (Fix xunit reporter, 2015-04-25) added a hard-coded empty object for the reporter options. This commit allows the user to override that with their own config, falling back to an empty object if the given value is falsy (for backwards compat with existing
Formatter(...)callers).If
Formatterwasn't already taking a runtime-options argument, I would have preferred:but we can't add
runnernamespacing to the existing options argument without breaking backwards compat or adding brittle heuristic translation. So I'm just adding a new option, where the reporter is namespaced to allow for other configurable aspects to also use the new argument (if we grow other configurable aspects in the future).