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.

Sign up for GitHub

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

Resolve #1 "Should not log succes message when app.option('silent') " #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Resolve #1 "Should not log succes message when app.option('silent') " #2

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion generator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

would you mind changing this to an if statement? I don't care if it's on one line or multiple, you can decide

Copy link
Member Author

Choose a reason for hiding this comment

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

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 to true, then do not log anything.
  • else, log as requested
  • add an option argument to the function, named force. If that option is set to true, ignore the value of app.option('silent')

I suggest to do an other pull request on that module.

Where app.log is set ?

Copy link
Member Author

Choose a reason for hiding this comment

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

In a such implementation, there will be zero changes made to generate-git unless upgrading the dependency for the external app.log object instantiation.

Copy link
Member

Choose a reason for hiding this comment

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

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?

cb();
}
});
Expand Down