-
Notifications
You must be signed in to change notification settings - Fork 56
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
Logging Reform. Integrate npmlog, char-spinner. Resolves gh-607 #768
Conversation
return callControllerWith(methodName, opts); | ||
}; | ||
} | ||
|
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.
No longer used. All calls consolidated to callControllerWith
Will review this tonight. |
ca1ab2d
to
3e45188
Compare
return new Promise(function(resolve, reject) { | ||
|
||
log.spinner.stop(); |
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.
Is this just making sure the spinner is defaulted to stop before starting it, in case it was not stopped by callControllerWith()
?
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.
Actually, this doesn't belong here—thanks for bringing my attention to it!
I say yes and agree with following the |
After reading the review comments, I decided to test another approach. As a result, I've pushed a completely revised version of this patch. |
Reviewing and smoke testing now. |
|
||
|
||
controller.installDrivers = function() { | ||
require('./tessel-install-drivers'); |
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.
Is this being required again for a reason? It's also on line 16:
var drivers = require('./tessel-install-drivers');
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.
Pfft. I never even noticed that. I just copied this from the old code.
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.
Fixed!
Overall, I love the simplification of spinner state management. This is going to be a very useful patch. LGTM 👍 |
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
…opriate locations. Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
…tions. Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
@HipsterBrown thank you so much for the several-days-long review, I appreciate your help enormously <3 |
This replaces our logging with npmlog and introduces a standardized integration of "char-spinner" which is used to indicate "something is happening".
This reformed logging system will allow making all logging levels configurable, ie. any type of logging can be enabled/disabled. Note that this capability is not yet exposed to the CLI directly. A few questions need to be answered:
t2 config
(and it should match npm configDemos:
--loglevel=debug
Notes:
http
ortrace
logs, but now that this is in place, I will spend some time implementing those.