Exposing the log flag and integrating with better logging plugin #357
Conversation
Codecov Report
@@ Coverage Diff @@
## master #357 +/- ##
==========================================
+ Coverage 99.24% 99.25% +<.01%
==========================================
Files 73 72 -1
Lines 1732 1737 +5
Branches 279 282 +3
==========================================
+ Hits 1719 1724 +5
Misses 13 13
Continue to review full report at Codecov.
|
Blackbaud-BobbyEarl
requested review from
Blackbaud-PaulCrowder and
Blackbaud-SteveBrush
|
|
||
| // Supporting a custom logging type of none | ||
| if (argv.log !== 'none') { | ||
| const log = skyPagesConfig.runtime.command === 'serve' ? 'compact' : 'expanded'; |
There was a problem hiding this comment.
Is the intent that skyux build should produce the "expanded" logs? I use skyux build -s often, and it add a ton of logs to the terminal. Curious if you thought about making the log level default to "compact" and put the option in skyuxconfig.json instead? This would allow control of the log level globally, but could be overwritten in command-specific config files.
There was a problem hiding this comment.
Apologies as that is a bit vague. compact and expanded produce the same logging, only difference is compact does it all on a single line. This is good for local development, but of any "CI" it would not be a good experience. https://github.com/dominique-mueller/simple-progress-webpack-plugin#compact
I am making the assumption here that if you're running build, you're probably on a CI. Obviously, you can run build locally, but seemed like the safer default to set.
Allowing the default to be set in skyuxconfig.json might not be a bad idea, as long as the command line overrides it. This would mean users could get verbose logging on the build machine without altering their build definition, which would be nice.
|
@Blackbaud-BobbyEarl Just one comment. I like that all logging goes through a single source now! |
|
@Blackbaud-SteveBrush I believe this is ready for another look. Since you first approved, I've solidifed on |
package.json
Outdated
| @@ -44,7 +44,12 @@ | |||
| "@angular/router": "4.3.6", | |||
| "@blackbaud/auth-client": "2.2.0", | |||
| "@blackbaud/skyux-lib-help": "1.1.0", | |||
| "@blackbaud/skyux-logger": "1.0.0", | |||
There was a problem hiding this comment.
I'm assuming we'll need to update this version once your patch gets in. Happy to approve the pull request once it gets released!
…into expose-log-flag
…ckbaud#357) * Exposing the log flag and integrating with better logging plugin * Using expanded logging in Travis * Adding support for custom --log none * Cleaned up logging * Fixed tests * Consistently using logger. Disabled native stats. * Cleaned up logging. Pointing to fork (until dep release) * Cleaned up defaults + tests * Fixed bug. Cleaned up logging * Incorporated feedback to use logColor, logLevel, and logFormat flags * Updated to @blackbaud/skyux-logger 1.0.1 * Removed failing test as it was testing internal logger functionality
This PR does a few things...
winstondirectly.--logColorflag. This will be useful in our VSTS build logs where color is not supported.showLevelfor thewinstontransport. This means no moreinfo: ...before every log entry.none(custom),minimal,compact,expanded, andverbose. See simple-progress-webpack-plugin.Currently pointing to my fork until @dominique-mueller has the opportunity to release.@blackbaud-johnly tagging you as this would require entries for the
--logLevel,--logColor, andlogFormatflags.