-
Notifications
You must be signed in to change notification settings - Fork 96
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
Pre release 0.4.2 changes #112
Conversation
2d74264
to
19f5826
Compare
Please do not remove function |
@develar oh sure! Let me drop that change quickly. |
@develar By the way, when would |
@sethlu Hmm... It seems you right. In any case I think it is better to leave to be sure that later it will be not exposed if password will be passed. |
@develar Thanks for the clarification... I'll rebase the commits tomorrow 😸 |
- Debuglog status before invoking callback function - Avoid possibly having callback function take in fulfilled value as error
e691f2f
to
3157e9c
Compare
if (debuglog.enabled) { | ||
debuglog(`Executing ${file} ${args == null ? '' : removePassword(args.join(' '))}`) | ||
} | ||
debuglog('Executing...', file, args && Array.isArray(args) ? removePassword(args.join(' ')) : '') |
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.
@develar will this work in the way you pictured?
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.
Yes. Why did you decide to not check debuglog.enabled
?
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.
@develar oh I thought if debuglog
is not enabled then nothing should be emitted from debuglog
... Or do we need to check debuglog.enabled
every time we try to log something?
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.
As we do here string concatenation, it is better to not do it if debug is not enabled. Ok — it doesn't affect performance noticeably :)
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.
@develar thanks for clarifying! That makes sense 😸 I'll update this later.
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.
@develar changes have already been applied in the commit amend.
3157e9c
to
18bfc7c
Compare
18bfc7c
to
0eddad6
Compare
If there are no other review, I will merge this branch in about 12 hours and push a new release 0.4.2. 😸 |
util.execFileAsync
#109 and overall code consistency