Resolve #1 "Should not log succes message when app.option('silent') " #2
Conversation
Result of the changeExample for the current PR's travis build with node carbon:
|
| @@ -59,7 +59,7 @@ module.exports = function(app, base) { | |||
| if (err && !/Command failed/.test(err.message)) { | |||
| cb(err); | |||
| } else { | |||
| app.log.success('first commit'); | |||
| !app.option('silent') && app.log.success('first commit'); | |||
There was a problem hiding this comment.
would you mind changing this to an if statement? I don't care if it's on one line or multiple, you can decide
There was a problem hiding this comment.
In fact I consider this more like a workaround.
I think a better approach would be to improve the app.log object.
But where is defined this object ? Is it from an external node module, like a base plugin ?
Let say that module exist, saying base-log. That object is probably an instance of a class named BaseLog or else, having the methods success(), info(), error(). Maybe each one depends on a inner generic function called with different arguments like color, toStringFn.
It is just some assumptions but if things work like that, I suggest the following changes:
- if
app.option('silent')is set totrue, then do not log anything. - else, log as requested
- add an option argument to the function, named
force. If that option is set totrue, ignore the value ofapp.option('silent')
I suggest to do an other pull request on that module.
Where app.log is set ?
There was a problem hiding this comment.
In a such implementation, there will be zero changes made to generate-git unless upgrading the dependency for the external app.log object instantiation.
There was a problem hiding this comment.
Sorry I missed this reply.
the logger is in Generate: https://github.com/generate/generate/blob/master/lib/plugins.js#L31.
Maybe we can refactor https://github.com/base/base-logger to be much simpler use that instead?
Resolve #1