Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

configure output #1387

Merged
merged 23 commits into from
Nov 17, 2020
Merged

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Oct 31, 2020

Pull Request

Problem

  • (consistency) we use console.error for errors and process.stderr.write for help displayed as an error
  • (consistency) we use console.error for errors and process.stdout.write for non-errors
  • no supported way to override output for unit testing or full custom implementations
  • wrap width for help in Commander v7 is using stdout.columns even when when displaying on stderr
  • no easy way to customise error output, like displaying in red

Issues: #1241 #1370 #1371

Solution

Add .configureOutput() to control output for unit tests or custom implementations. By default use process.stderr.write rather than console.error for displaying errors. Includes outputError for easy customisation.

I do not think the out/err customisation will be a widely used feature, but improves support for full custom implementations and unit testing with the pair of .configureOutput() and .exitOveride().

The default implementations are:

this._outputConfiguration = {
  // Routines for where output is going, stdout or stderr
  writeOut: (str) => process.stdout.write(str),
  writeErr: (str) => process.stderr.write(str),
  // columns is used for wrapping the help
  getOutColumns: () => process.stdout.isTTY ? process.stdout.columns : undefined,
  getErrColumns: () => process.stderr.isTTY ? process.stderr.columns : undefined,
  // routines for what is being written out
  outputError: (str, write) => write(str) // used for displaying errors, and not used for displaying help
};

Note: getOutColumns and getErrColumns later renamed to getOutHelpWidth and getErrHelpWidth. See #1396

To Do

  • README
  • TypeScript
  • tests for outputError
  • example(s)
  • JSDoc

Release Notes

  • [Add] .configureOutput() to modify use of stdout and stderr or customise display of errors (configure output #1387)

@shadowspawn

This comment has been minimized.

@cravler
Copy link
Contributor

cravler commented Nov 4, 2020

I think, we need to add code for writeOut/writeErr, so then I can do:

commander.configureOutput({
    writeErr: (code, message) => {
        if ('commander.help' !== code) {
            const prefix = '[' + (new Date()).toISOString() + '] ';
            const applyStyle = str => '\x1b[31m' + str + '\x1b[0m';
            message = applyStyle(prefix + message);
        }
        process.stderr.write(message);
    },
});

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Nov 5, 2020

I was not designing this form of configureOutput to provide a hook for styling the non-help error output, and had added the bottleneck _displayError for that purpose.

However, the suggestion to add code parameter to writeErr() has some interesting parallels with the exit call this._exit(exitCode, code, message); which I hadn't seriously considered before.

@shadowspawn
Copy link
Collaborator Author

I decided adding code was not a good match with .exitOverride() as the write routines are also used for strings returned by addHelpText().

Instead, I am trying an outputError routine for customising "what" is displayed rather than "where" it is displayed. The write argument can be ignored, it is passed writeErr as a convenience. Defaults:

    this._outputConfiguration = {
      // Routines for where output is going, stdout or stderr
      writeOut: (str) => process.stdout.write(str),
      writeErr: (str) => process.stderr.write(str),
      // columns is used for wrapping the help
      getOutColumns: () => process.stdout.isTTY ? process.stdout.columns : undefined,
      getErrColumns: () => process.stderr.isTTY ? process.stderr.columns : undefined,
      // routines for what is being written out
      outputError: (str, write) => write(str) // used for displaying errors, and not used for displaying help
    };

(To give an idea of the pattern, there could be an outputVersion too. I'm not currently planning to add one, but have the pattern in mind in case of future extension!)

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Nov 7, 2020

So @cravler example could be:

commander.configureOutput({
    outputError: (message, write) => {
        const prefix = '[' + (new Date()).toISOString() + '] ';
        const applyStyle = str => '\x1b[31m' + str + '\x1b[0m';
        write(applyStyle(prefix + message));
    },
});

Does that look useful?

@shadowspawn
Copy link
Collaborator Author

Another naming possibility for "getOutColumns". yargs has a .wrap() routine to set the wrapping columns and a .terminalWidth() method: https://yargs.js.org/docs/#api-reference-wrapcolumns

@shadowspawn shadowspawn mentioned this pull request Nov 10, 2020
@shadowspawn shadowspawn marked this pull request as ready for review November 14, 2020 05:08
@shadowspawn shadowspawn changed the title WIP: configure output configure output Nov 15, 2020
Readme.md Outdated
writeOut: (str) => process.stdout.write(`[OUT] ${str}`),
writeErr: (str) => process.stdout.write(`[ERR] ${str}`),
// Output errors in red.
outputError: (str, write) => write(red(str))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I found it difficult to understand the red function because it came out of nowhere.

How about including the following in the readme code examples as well?

function red(str) {
  return `\x1b[31m${str}\x1b[0m`;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good feedback, will do. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Switched name to errorColor (which was what I called this in one of my own programs!) and added implementation to README as suggested.

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@shadowspawn shadowspawn added this to the v7.0.0 milestone Nov 17, 2020
@shadowspawn
Copy link
Collaborator Author

Thanks @abetomo

@shadowspawn shadowspawn merged commit ed7f13e into tj:release/7.x Nov 17, 2020
@shadowspawn shadowspawn deleted the feature/output-override branch November 17, 2020 06:44
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Nov 17, 2020
@shadowspawn
Copy link
Collaborator Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants