-
-
Notifications
You must be signed in to change notification settings - Fork 107
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
Use theming-log for logging. #161
Changes from 6 commits
be0955c
9034164
2d3a80d
a39ab8c
3a95fd3
741ff5a
a4bfdd5
237cb58
ba976ab
d8e01e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,22 +3,25 @@ | |
var fs = require('fs'); | ||
var path = require('path'); | ||
var log = require('gulplog'); | ||
|
||
var Liftoff = require('liftoff'); | ||
var interpret = require('interpret'); | ||
var v8flags = require('v8flags'); | ||
var findRange = require('semver-greatest-satisfied-range'); | ||
var chalk = require('chalk'); | ||
var format = require('theming-log').format; | ||
|
||
var exit = require('./lib/shared/exit'); | ||
var tildify = require('./lib/shared/tildify'); | ||
var makeTitle = require('./lib/shared/make-title'); | ||
var parser = require('./lib/shared/options/parser'); | ||
var makeHelp = require('./lib/shared/options/make-help'); | ||
var completion = require('./lib/shared/completion'); | ||
var cliVersion = require('./package.json').version; | ||
var toConsole = require('./lib/shared/log/to-console'); | ||
var theme = require('./lib/shared/log/theme'); | ||
var msgs = require('./lib/shared/log/messages'); | ||
|
||
var mergeProjectAndUserHomeConfigs = require('./lib/shared/config/merge-configs'); | ||
var overrideEnvFlagsByConfigAndCliOpts = require('./lib/shared/config/env-flags'); | ||
var overrideEnvByConfigAndCliOpts = require('./lib/shared/config/env-config'); | ||
|
||
// Get supported ranges | ||
var ranges = fs.readdirSync(path.join(__dirname, '/lib/versioned/')); | ||
|
@@ -52,25 +55,24 @@ var cli = new Liftoff({ | |
}, | ||
}); | ||
|
||
var opts = parser.argv; | ||
var opts = {}; | ||
var optsErr; | ||
try { | ||
opts = parser.argv; | ||
} catch (e) { | ||
optsErr = e; | ||
} | ||
|
||
cli.on('preload:before', function(name) { | ||
log.info('Preloading external module:', chalk.magenta(name)); | ||
log.info(msgs.info.preloadBefore, name); | ||
}); | ||
|
||
cli.on('preload:success', function(name) { | ||
log.info('Preloaded external module:', chalk.magenta(name)); | ||
log.info(msgs.info.preloadSuccess, name); | ||
}); | ||
|
||
cli.on('preload:failure', function(name, error) { | ||
log.warn( | ||
chalk.yellow('Failed to preload external module:'), | ||
chalk.magenta(name) | ||
); | ||
/* istanbul ignore else */ | ||
if (error) { | ||
log.warn(chalk.yellow(error.toString())); | ||
} | ||
log.warn(msgs.warn.preloadFailure, name, Boolean(error), error.toString()); | ||
}); | ||
|
||
cli.on('loader:success', function(name) { | ||
|
@@ -79,26 +81,16 @@ cli.on('loader:success', function(name) { | |
// However, we don't want to show the mjs-stub loader in the logs | ||
/* istanbul ignore else */ | ||
if (path.basename(name, '.js') !== 'mjs-stub') { | ||
log.info('Loaded external module:', chalk.magenta(name)); | ||
log.info(msgs.info.loaderSuccess, name); | ||
} | ||
}); | ||
|
||
cli.on('loader:failure', function(name, error) { | ||
log.warn( | ||
chalk.yellow('Failed to load external module:'), | ||
chalk.magenta(name) | ||
); | ||
/* istanbul ignore else */ | ||
if (error) { | ||
log.warn(chalk.yellow(error.toString())); | ||
} | ||
log.warn(msgs.warn.loaderFailure, name, Boolean(error), error.toString()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like combining the preload message failure and the error printing. Can we keep them separate? |
||
}); | ||
|
||
cli.on('respawn', function(flags, child) { | ||
var nodeFlags = chalk.magenta(flags.join(', ')); | ||
var pid = chalk.magenta(child.pid); | ||
log.info('Node flags detected:', nodeFlags); | ||
log.info('Respawned to PID:', pid); | ||
log.info(msgs.info.respawn, flags.join(', '), child.pid); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These should not be combined, one message is about detecting node flags and one if about the respawn. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that users configuring this would consider these two lines as one. |
||
}); | ||
|
||
function run() { | ||
|
@@ -114,7 +106,7 @@ module.exports = run; | |
|
||
function onPrepare(env) { | ||
var cfg = mergeProjectAndUserHomeConfigs(env); | ||
env = overrideEnvFlagsByConfigAndCliOpts(env, cfg, opts); | ||
env = overrideEnvByConfigAndCliOpts(env, cfg, opts); | ||
|
||
// Set up event listeners for logging again after configuring. | ||
toConsole(log, env.config.flags); | ||
|
@@ -132,69 +124,56 @@ function onExecute(env) { | |
process.env.UNDERTAKER_SETTLE = 'true'; | ||
} | ||
|
||
if (optsErr) { | ||
log.error(msgs.error.failToParseCliOpts, optsErr.message); | ||
makeHelp(parser).showHelp(console.error); | ||
exit(1); | ||
} | ||
Comment on lines
+125
to
+129
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part is needed against an error process of yargs for invalid option. |
||
if (env.config.flags.help) { | ||
parser.showHelp(console.log); | ||
makeHelp(parser).showHelp(console.log); | ||
exit(0); | ||
} | ||
|
||
// Anything that needs to print outside of the logging mechanism should use console.log | ||
if (env.config.flags.version) { | ||
console.log('CLI version:', cliVersion); | ||
console.log('Local version:', env.modulePackage.version || 'Unknown'); | ||
var gulpVersion = env.modulePackage.version || 'Unknown'; | ||
console.log(format(theme, msgs.info.version, cliVersion, gulpVersion)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we want to allow this to be configured. We ask for this specific information in our bug reports. Anywhere we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Understood. I think that's reasonable. |
||
exit(0); | ||
} | ||
|
||
if (!env.modulePath) { | ||
/* istanbul ignore next */ | ||
var missingNodeModules = | ||
fs.existsSync(path.join(env.cwd, 'package.json')) | ||
&& !fs.existsSync(path.join(env.cwd, 'node_modules')); | ||
|
||
/* istanbul ignore next */ | ||
var missingGulpMessage = | ||
missingNodeModules | ||
? 'Local modules not found in' | ||
: 'Local gulp not found in'; | ||
log.error( | ||
chalk.red(missingGulpMessage), | ||
chalk.magenta(tildify(env.cwd)) | ||
); | ||
var hasYarn = fs.existsSync(path.join(env.cwd, 'yarn.lock')); | ||
/* istanbul ignore next */ | ||
var installCommand = | ||
missingNodeModules | ||
? hasYarn | ||
? 'yarn install' | ||
: 'npm install' | ||
: hasYarn | ||
? 'yarn add gulp' | ||
: 'npm install gulp'; | ||
log.error(chalk.red('Try running: ' + installCommand)); | ||
var hasNpm = !hasYarn; | ||
|
||
if (missingNodeModules) { | ||
log.error(msgs.error.nodeModulesNotFound, tildify(env.cwd), hasYarn, hasNpm); | ||
} else { | ||
log.error(msgs.error.gulpNotFound, tildify(env.cwd), hasYarn, hasNpm); | ||
Comment on lines
+151
to
+153
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These seem strange to me right now. I need to think more. |
||
} | ||
exit(1); | ||
} | ||
|
||
if (!env.configPath) { | ||
log.error(chalk.red('No gulpfile found')); | ||
log.error(msgs.error.gulpfileNotFound); | ||
exit(1); | ||
} | ||
|
||
// Chdir before requiring gulpfile to make sure | ||
// we let them chdir as needed | ||
if (process.cwd() !== env.cwd) { | ||
process.chdir(env.cwd); | ||
log.info( | ||
'Working directory changed to', | ||
chalk.magenta(tildify(env.cwd)) | ||
); | ||
log.info(msgs.info.cwdChanged, tildify(env.cwd)); | ||
} | ||
|
||
// Find the correct CLI version to run | ||
var range = findRange(env.modulePackage.version, ranges); | ||
|
||
if (!range) { | ||
log.error( | ||
chalk.red('Unsupported gulp version', env.modulePackage.version) | ||
); | ||
log.error(msgs.error.badGulpVersion, env.modulePackage.version); | ||
exit(1); | ||
} | ||
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is for providing shell completions, we probably don't need to use formatting in here at all. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also aim to enable multilingualization support with this change. I want to make it possible to configure all messages generated by gulp-cli. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,21 +2,21 @@ | |
|
||
var fs = require('fs'); | ||
var path = require('path'); | ||
var format = require('theming-log').format; | ||
|
||
var theme = require('./log/theme'); | ||
var msgs = require('./log/messages'); | ||
|
||
module.exports = function(name) { | ||
if (typeof name !== 'string') { | ||
throw new Error('Missing completion type'); | ||
throw new Error(format(theme, msgs.error.noCompletionType)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we format the message for a thrown error? |
||
} | ||
var file = path.join(__dirname, '../../completion', name); | ||
try { | ||
console.log(fs.readFileSync(file, 'utf8')); | ||
process.exit(0); | ||
} catch (err) { | ||
console.log( | ||
'echo "gulp autocompletion rules for', | ||
'\'' + name + '\'', | ||
'not found"' | ||
); | ||
console.log(format(theme, msgs.error.unknownCompletionType, name)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this uses |
||
process.exit(5); | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ var path = require('path'); | |
|
||
function mergeConfigs(env) { | ||
var cfg = {}; | ||
/* istanbul ignore if */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For increasing coverage. It's impossible to test a config file in user home. |
||
if (env.configFiles.userHome) { | ||
copyConfig(env.config.userHome, cfg, env.configFiles.userHome); | ||
} | ||
|
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.
I don't like combining the preload message failure and the error printing. Can we keep them separate?