Add option to exclude -g argument from default update message
#105
Conversation
index.js
Outdated
| @@ -109,8 +109,10 @@ class UpdateNotifier { | |||
|
|
|||
| opts = opts || {}; | |||
|
|
|||
| opts.isGlobal = typeof opts.isGlobal === 'boolean' ? opts.global : true; | |||
There was a problem hiding this comment.
Would prefer using Object.assign here. You could then drop line 110 too.
opts = Object.assign({global: true}, opts);
test.js
Outdated
| ' ╰────────────────────────────────────────────────╯', | ||
| '', | ||
| '' | ||
| ].join('\n')); |
There was a problem hiding this comment.
No need to test the whole output. Having many of these will make it hard to change for example the box style later. Instead just do a indexOf check for Run npm i update-notifier-tester to update, after running it through stripAnsi of course.
There was a problem hiding this comment.
I agree. Change coming!
test.js
Outdated
| latest: '1.0.0' | ||
| }; | ||
| } | ||
| util.inherits(Control, updateNotifier.UpdateNotifier); |
There was a problem hiding this comment.
This is now duplicated. Can you make it out of these test function and to the top of the function.
Bonus points for making it a ES2015 class instead.
There was a problem hiding this comment.
One step at a time! I'll make the refactor and perhaps do the class conversion as a separate PR. I don't wanna mix up to many changes at once!
readme.md
Outdated
| Type: `boolean`<br> | ||
| Default: `true` | ||
|
|
||
| Include the `-g` argument in the default message's `npm i` recommendation. This is ignored if you supply your own `message`. |
There was a problem hiding this comment.
Maybe shortly explain when it can be useful to set this to false? Like you did in the PR description.
There was a problem hiding this comment.
I didn't want to make the description too long (as the rest were quite short), but if you feel like it's a good explanation, I'll work it in there. Thanks so much for all of this feedback!
sindresorhus
changed the title
-g argument from default update message.-g argument from default update message
… into a bit more detail on the new option.
|
@sindresorhus that's your recommendations rolled in 👍 |
|
Looks very good. Thank you @colingourlay :) |
In cases where your CLI package also serves a secondary purpose as a dependency of a project (e.g. the new crop of single-dependency toolkits such as nwb), the default update message shouldn't be recommending a global npm install.
With this new option (
isGlobal), the default behaviour stays the same, but it is now configurable. Of course, this has always been possible when using themessageargument, but it's a shame for authors to lose out on the pretty boxen output when they just want to change a flag.I've updated the tests to ensure this output is correct, but they could use a little refactoring now that more than one test uses a similar
UpdateNotifiersub-class.