-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add support for "local" and global help:header, help:body and help:footer #1267
Changes from all commits
f34c7d9
4a32a73
5e09664
0f57932
69cdff6
e69b077
a3bf3d2
51dbcab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
#!/usr/bin/env node | ||
|
||
// const program = require('commander'); // (normal include) | ||
const program = require('../'); // include commander in git clone of commander repo | ||
const { commanderGlobalEmitter } = require('../') | ||
|
||
commanderGlobalEmitter | ||
.on('help:header', () => { | ||
console.log('Global Header\n'); | ||
}) | ||
.on('help:body', (instance) => { | ||
console.log('++++++global++++++'); | ||
console.log(instance.helpInformation().replace(/\n$/, '\n++++++global++++++\n')); | ||
}) | ||
.on('help:footer', () => { | ||
console.log(`The End.`); | ||
}) | ||
|
||
|
||
program | ||
.version('0.0.1') | ||
.option('-b, --bar', 'enable some bar') | ||
|
||
program | ||
.on('help:header', () => { | ||
console.log('Custom Header\n'); | ||
}) | ||
.on('help:body', (instance) => { | ||
console.log('------local------'); | ||
console.log(instance.helpInformation().replace(/\n$/, '\n------local------\n')); | ||
}) | ||
.on('help:footer', () => { | ||
console.log(`Commander - ${new Date().getFullYear()}`); | ||
}) | ||
.command('foo') | ||
.action(() => {}) | ||
|
||
program | ||
.parse(process.argv); | ||
|
||
/** | ||
* examples/help-global-listeners -h | ||
* | ||
* Should display both global and "local" emitter messages | ||
*/ | ||
|
||
/** | ||
* examples/help-global-listeners foo -h | ||
* | ||
* Should display only global emitter messages | ||
*/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
#!/usr/bin/env node | ||
|
||
// const program = require('commander'); // (normal include) | ||
const program = require('../'); // include commander in git clone of commander repo | ||
|
||
program | ||
.version('0.0.1') | ||
.option('-b, --bar', 'enable some bar') | ||
.on('help:header', () => { | ||
console.log('Custom Header\n'); | ||
}) | ||
.on('help:body', (instance) => { | ||
console.log('------------'); | ||
console.log(instance.helpInformation().replace(/\n$/, '\n------------\n')); | ||
}) | ||
.on('help:footer', () => { | ||
console.log(`Commander - ${new Date().getFullYear()}`); | ||
}) | ||
.parse(process.argv); | ||
|
||
if (!program.args.length) program.help(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ const EventEmitter = require('events').EventEmitter; | |
const spawn = require('child_process').spawn; | ||
const path = require('path'); | ||
const fs = require('fs'); | ||
const globalEventEmitter = new EventEmitter(); | ||
|
||
// @ts-check | ||
|
||
|
@@ -128,6 +129,9 @@ class Command extends EventEmitter { | |
this._helpCommandName = 'help'; | ||
this._helpCommandnameAndArgs = 'help [command]'; | ||
this._helpCommandDescription = 'display help for command'; | ||
this._helpHeader = 'help:header'; | ||
this._helpBody = 'help:body'; | ||
this._helpFooter = 'help:footer'; | ||
Comment on lines
+132
to
+134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These can be used as literals rather than having a property for them. You were probably copying the pattern for |
||
} | ||
|
||
/** | ||
|
@@ -1530,12 +1534,23 @@ class Command extends EventEmitter { | |
return passthru; | ||
}; | ||
} | ||
globalEventEmitter.emit(this._helpHeader, this); | ||
this.emit(this._helpHeader, this); | ||
const cbOutput = cb(this.helpInformation()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In theory we should try and include or make available cb in the processing of the listener too, but that callback is a pattern I would like to deprecate. Instead, the processing of the callback can shift into the non-override body block since no need to call if not being displayed. |
||
if (typeof cbOutput !== 'string' && !Buffer.isBuffer(cbOutput)) { | ||
throw new Error('outputHelp callback must return a string or a Buffer'); | ||
} | ||
process.stdout.write(cbOutput); | ||
this.emit(this._helpLongFlag); | ||
const globalBodyListeners = globalEventEmitter.listeners(this._helpBody); | ||
const bodyListeners = this.listeners(this._helpBody); | ||
Comment on lines
+1543
to
+1544
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have been moving to a different pattern for testing listeners, since we don't call the listener directly we do not need the list. Just check the count. An example of pattern, not a direct code replacement: if (this.listenerCount('help:body')) {
this.emit('help:body', this);
} else {
process.stdout.write(cbOutput);
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops. Yeah is missed that. 😅 |
||
if (bodyListeners.length === 0 && globalBodyListeners.length === 0) { | ||
process.stdout.write(cbOutput); | ||
} else { | ||
globalEventEmitter.emit(this._helpBody, this); | ||
this.emit(this._helpBody, this); | ||
Comment on lines
+1548
to
+1549
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think only one of the body events should get emitted, the local one in preference to the global one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think it might be better too, I was in doubt of what to do there, thanks :) |
||
} | ||
this.emit(this._helpLongFlag); // Leave .on('--help') in for compatibility | ||
this.emit(this._helpFooter, this); | ||
globalEventEmitter.emit(this._helpFooter, this); | ||
}; | ||
|
||
/** | ||
|
@@ -1604,6 +1619,12 @@ exports.Command = Command; | |
exports.Option = Option; | ||
exports.CommanderError = CommanderError; | ||
|
||
/** | ||
* Expose Global Emiter | ||
*/ | ||
|
||
exports.commanderGlobalEmitter = globalEventEmitter; | ||
|
||
/** | ||
* Camel-case the given `flag` | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,124 @@ | ||
const commander = require('../'); | ||
const { commanderGlobalEmitter } = require('../'); | ||
|
||
jest.spyOn(global.console, 'log').mockImplementation(); | ||
|
||
// Testing help listeners, both "local" and global | ||
|
||
// Avoid doing many full format tests as will be broken by any layout changes! | ||
test('when listening for the global help:body the default help command should be overridden', () => { | ||
const program = new commander.Command(); | ||
|
||
commanderGlobalEmitter | ||
.on('help:body', (instance) => { | ||
console.log(`------------ | ||
${instance.helpInformation().replace(/\n$/, '\n------------\n')}`); | ||
}); | ||
|
||
program | ||
.command('my-command <file>'); | ||
const expectedHelpInformation = | ||
`------------ | ||
Usage: test [options] [command] | ||
|
||
Options: | ||
-h, --help display help for command | ||
|
||
Commands: | ||
my-command <file> | ||
help [command] display help for command | ||
------------ | ||
`; | ||
|
||
program.name('test'); | ||
program.outputHelp(); | ||
expect(global.console.log).toHaveBeenCalledWith(expectedHelpInformation); | ||
}); | ||
|
||
test('when listening for the global help:header the listener should be called correctly', () => { | ||
const program = new commander.Command(); | ||
const listenerCall = jest.fn(); | ||
|
||
commanderGlobalEmitter | ||
.on('help:header', listenerCall); | ||
|
||
program | ||
.command('my-command <file>'); | ||
|
||
program.outputHelp(); | ||
expect(listenerCall).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
test('when listening for the global help:footer the listener should be called correctly', () => { | ||
const program = new commander.Command(); | ||
const listenerCall = jest.fn(); | ||
|
||
commanderGlobalEmitter | ||
.on('help:footer', listenerCall); | ||
|
||
program | ||
.command('my-command <file>'); | ||
|
||
program.outputHelp(); | ||
expect(listenerCall).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
test('when listening for global help:header, help:body and help:footer they should be called in the correct order', () => { | ||
const program = new commander.Command(); | ||
const callOrder = []; | ||
|
||
commanderGlobalEmitter | ||
.on('help:footer', () => { | ||
callOrder.push('help:footer'); | ||
}) | ||
.on('help:header', () => { | ||
callOrder.push('help:header'); | ||
}) | ||
.on('help:body', () => { | ||
callOrder.push('help:body'); | ||
}); | ||
|
||
program | ||
.command('my-command <file>'); | ||
|
||
program.outputHelp(); | ||
expect(callOrder[0]).toBe('help:header'); | ||
expect(callOrder[1]).toBe('help:body'); | ||
expect(callOrder[2]).toBe('help:footer'); | ||
}); | ||
|
||
test('when listening for local and global help:header, help:body and help:footer they should be called in the correct order', () => { | ||
const program = new commander.Command(); | ||
const callOrder = []; | ||
|
||
commanderGlobalEmitter | ||
.on('help:footer', () => { | ||
callOrder.push('global:help:footer'); | ||
}) | ||
.on('help:header', () => { | ||
callOrder.push('global:help:header'); | ||
}) | ||
.on('help:body', () => { | ||
callOrder.push('global:help:body'); | ||
}); | ||
|
||
program | ||
.on('help:footer', () => { | ||
callOrder.push('help:footer'); | ||
}) | ||
.on('help:header', () => { | ||
callOrder.push('help:header'); | ||
}) | ||
.on('help:body', () => { | ||
callOrder.push('help:body'); | ||
}) | ||
.command('my-command <file>'); | ||
|
||
program.outputHelp(); | ||
expect(callOrder[0]).toBe('global:help:header'); | ||
expect(callOrder[1]).toBe('help:header'); | ||
expect(callOrder[2]).toBe('global:help:body'); | ||
expect(callOrder[3]).toBe('help:body'); | ||
expect(callOrder[4]).toBe('help:footer'); | ||
expect(callOrder[5]).toBe('global:help:footer'); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
const commander = require('../'); | ||
|
||
jest.spyOn(global.console, 'log').mockImplementation(); | ||
|
||
// Testing help listeners, both "local" and global | ||
|
||
// Avoid doing many full format tests as will be broken by any layout changes! | ||
test('when listening for the help:body the default help command should be overridden', () => { | ||
const program = new commander.Command(); | ||
program | ||
.on('help:body', (instance) => { | ||
console.log(`------------ | ||
${instance.helpInformation().replace(/\n$/, '\n------------\n')}`); | ||
}) | ||
.command('my-command <file>'); | ||
const expectedHelpInformation = | ||
`------------ | ||
Usage: test [options] [command] | ||
|
||
Options: | ||
-h, --help display help for command | ||
|
||
Commands: | ||
my-command <file> | ||
help [command] display help for command | ||
------------ | ||
`; | ||
|
||
program.name('test'); | ||
program.outputHelp(); | ||
expect(global.console.log).toHaveBeenCalledWith(expectedHelpInformation); | ||
}); | ||
|
||
test('when listening for the help:header the listener should be called correctly', () => { | ||
const program = new commander.Command(); | ||
const listenerCall = jest.fn(); | ||
program | ||
.on('help:header', listenerCall) | ||
.on('help:body', () => {}) // to prevent printing to console | ||
.command('my-command <file>'); | ||
|
||
program.outputHelp(); | ||
expect(listenerCall).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
test('when listening for the help:footer the listener should be called correctly', () => { | ||
const program = new commander.Command(); | ||
const listenerCall = jest.fn(); | ||
program | ||
.on('help:footer', listenerCall) | ||
.on('help:body', () => {}) // to prevent printing to console | ||
.command('my-command <file>'); | ||
|
||
program.outputHelp(); | ||
expect(listenerCall).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
test('when listening for help:header, help:body and help:footer they should be called in the correct order', () => { | ||
const program = new commander.Command(); | ||
const callOrder = []; | ||
program | ||
.on('help:footer', () => { | ||
callOrder.push('help:footer'); | ||
}) | ||
.on('help:header', () => { | ||
callOrder.push('help:header'); | ||
}) | ||
.on('help:body', () => { | ||
callOrder.push('help:body'); | ||
}) | ||
.command('my-command <file>'); | ||
|
||
program.outputHelp(); | ||
expect(callOrder[0]).toBe('help:header'); | ||
expect(callOrder[1]).toBe('help:body'); | ||
expect(callOrder[2]).toBe('help:footer'); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,15 @@ declare namespace commander { | |
from: 'node' | 'electron' | 'user'; | ||
} | ||
|
||
// (string & {}) is sort of a hack so we able to show the string types autocompletion | ||
// as well as allow any string to be used | ||
export type Event = | ||
| 'help:header' | ||
| 'help:body' | ||
| 'help:footer' | ||
| (string & {}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does indeed seem to work in Visual Studio Code. Do you have any documentation or a source for what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went to hunt for some sort of explanation on this but couldn't find any. So I'll try to explain what I believe happens. An empty interface interface Empty {}
const n:Empty = 0
const s:Empty = 'a'
const a:Empty = []
const o:Empty = {}
const t:Empty = true
const f:Empty = false
const sy:Empty = Symbol()
const u:Empty = undefined //error
const nu:Empty = null //error So the typescript intersection When you declare The two drawbacks I can find or thing of are: In some typeguard functions like the one below, it will throw an error, though it is easily bypass-able. function foo(arg: 'a' | 'b' | (string & {})): 'a' | undefined {
if (arg === 'a') {
return arg; // Type '(string & {}) | "a"' is not assignable to type '"a"'
//return arg as 'a'; // this works
}
return
} The other drawback might be that this could be patched in the future, since I couldn't find any documentation or explanation for it. Though I suspect if patched, it would behave just like a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An alternative to this would be to overload the "on" function with on(event: 'help:header' | 'help:body' | 'help:footer' | symbol, listener: (...args: any[]) => void): this;
on(event: string, listener: (...args: any[]) => void): this; |
||
| symbol | ||
|
||
interface Command { | ||
[key: string]: any; // options as properties | ||
|
||
|
@@ -353,7 +362,7 @@ declare namespace commander { | |
* console.log('See web site for more information.'); | ||
* }); | ||
*/ | ||
on(event: string | symbol, listener: (...args: any[]) => void): this; | ||
on(event: Event, listener: (...args: any[]) => void): this; | ||
} | ||
type CommandConstructor = new (name?: string) => Command; | ||
|
||
|
@@ -371,13 +380,26 @@ declare namespace commander { | |
unknown: string[]; | ||
} | ||
|
||
interface CommanderGlobalEmitter { | ||
/** | ||
* Add a listener (callback) for when events occur. (Implemented using EventEmitter.) | ||
* | ||
* @example | ||
* program | ||
* .on('help:body', () -> { | ||
* console.log('See web site for more information.'); | ||
* }); | ||
*/ | ||
on(event: Event, listener: (...args: any[]) => void): this; | ||
} | ||
|
||
interface CommanderStatic extends Command { | ||
program: Command; | ||
commanderGlobalEmitter: CommanderGlobalEmitter; | ||
Command: CommandConstructor; | ||
Option: OptionConstructor; | ||
CommanderError: CommanderErrorConstructor; | ||
} | ||
|
||
} | ||
|
||
// Declaring namespace AND global | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The replace and
\n
are a bit confusing here. I eventually recalled thathelpInformation()
has a trailing return for legacy reasons because it is used withprocess.stdout.write
. I suggest a simpler more symmetrical pattern like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, what you suggested was close to my first approach I believe, but I wanted the output to be neat. But indeed it is much better to read.