-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[Feature] Log stdout when commands fail. Fixes #343. #373
Conversation
The decorator spec requires that a function decorator either: - returns a new /descriptor/ for the property, or - changes the fields on the given descriptor The logify* methods instead returned a new function implementation, which is not correct, and caused the decorations to be silently ignored. decorator spec: https://github.com/wycats/javascript-decorators#detailed-design function decorator example: https://github.com/jayphelps/core-decorators.js/blob/master/src/deprecate.js This commit changes logifySync and logifyAsync in the following ways: - they now have no return value, and instead modify the descriptor - they now must be called with no arguments so they can properly capture the `this` value (without this, the function cannot find the logger instance) - they now log at 'verbose', because they are extremely verbose - remove 'error' flag from non-error logging - they truncate their serialized arguments to (arbitrarily) 100 characters because otherwise they they OOM the node process Additionally, one async method was erroneously decorated with logifySync.
ChildProcessUtilities now returns stdout as the second argument of a callback. logifyAsync will check for the presence of a second argument and log it out in the case of an error, too.
Should we be combining |
I hadn't thought of that, but yeah, I think that's the right thing to do. Will take another look Thursday. |
@thejameskyle finally got around to making those changes. |
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.
Looks fine to me.
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.
woohoo! thanks @seansfkelley
This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
ChildProcessUtilities now returns stdout as the second argument of
a callback. logifyAsync will check for the presence of a second
argument and log it out in the case of an error, too.