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

Restrict config properties (Implementation for Issue #92) #94

Merged
merged 7 commits into from Apr 22, 2017

Conversation

Projects
None yet
2 participants
@sttk
Member

sttk commented Sep 4, 2016

This PR is for the issue #92.

I only implemented about the properties: description, flags.help, and flags.gulpfile first.

The points that I modified from #90 are as follows:

  1. Config properties by cli-flags take precedence over them by config files.

    I made a mistake in the previous implementation to merge properties of config files over opts properties, so config files took precedence over command line flags. I modified it.

    Since it would be confusing to control to change or not opts properties at the time of merging, the current code evacuates opts properties specified by cli-flags before merging, merge with config by config files, and merge with the evacuated properties at the end.

  2. Add an object disp for display settings.

    This object is used for outputing description for gulpfile and will be used for various display configurations.

    For this object, the api of the versioned cli modules in lib/versioned/*/index.js are added 3rd argument.

  3. Add validations for config properties of each config file.

    And if a validation fails, outputs the error and quits immediately.

  4. Add convert for config properties of each config file.

    Because it is needed to convert a property value into an absolute path with a base directory path if the property value is a path string.

  5. Add env to the merged targets because env.cwd, env.configPath and env.require are used for configuration of cwd, gulpfile and require.

@sttk

This comment has been minimized.

Member

sttk commented Sep 4, 2016

In addition, we should consider about following points:

  1. Specifying --cwd or --gulpfile affect each other. Is it OK to set flags.cwd or flags.gulpfile independently about specifying by config file?
  2. --require could be specified multiple values. Should the value of flags.require be able to set a string or an array? And Should the value by config files replace or append?
  3. Is it OK that a base path of flags.cwd and flags.gulpfile are a directory of a .gulp.* file?
index.js Outdated
function customizeByEachConfig(config, opts, env, disp, filePath) {
try {
var config = require(filePath);

This comment has been minimized.

@sttk

sttk Sep 5, 2016

Member

I found that this line is redundant with line 182. I'll remove this line or line 182.

@sttk

This comment has been minimized.

Member

sttk commented Sep 6, 2016

I've removed the redundant code.

@phated

This comment has been minimized.

Member

phated commented Sep 6, 2016

@sttk any idea why the tests failed on node 6?

@sttk

This comment has been minimized.

Member

sttk commented Sep 6, 2016

The cause of this error is seemed to be an accidental timeout of the test for --completion and it is thought that this is not related to this PR.

Actually, the same test for my repository is successful.

https://travis-ci.org/sttk/gulp-cli/builds/157911810

@phated

This comment has been minimized.

Member

phated commented Sep 15, 2016

@sttk Sorry for not having a chance to fully review this yet. I don't think description should be placed under a disp key and should remain at the top level. You can drop disp completely for now.

@sttk

This comment has been minimized.

Member

sttk commented Sep 17, 2016

@phated disp is not a key of config or opts but an inner object to pass to versioned runners. Since the description key places under opts in #90 implementation, this key can be also specified by unspecific --description cli-flag. disp is for preventing it.

@sttk

This comment has been minimized.

Member

sttk commented Dec 4, 2016

I re-created the code for this PR.

This code supports and restricts only the config properties: description, flags.silent and flags.gulpfile first.

One problem about the property flag.silent occurs. Before loading config files, which is executed at the top of a callback function of Liftoff.launch, configuration of logging is already done in toConsole function.

Tentatively, I moved toConsole function after the loading, and for the requiring logs, which are output before the loading, I used fancy-log directly .

@phated

This comment has been minimized.

Member

phated commented Dec 14, 2016

@sttk ugh, sorry that I still haven't had a chance to review this PR. Life has been hectic for me. I will try to review soon.

@phated

This comment has been minimized.

Member

phated commented Dec 27, 2016

@sttk this will need to be updated after I merge the mocha/expect changes.

@phated

This comment has been minimized.

Member

phated commented Jan 6, 2017

Feel free to rebase/update this based on the merge of #99

@sttk

This comment has been minimized.

Member

sttk commented Jan 7, 2017

@phated I'll update this PR.

@sttk

This comment has been minimized.

Member

sttk commented Jan 7, 2017

@phated I finished updating this PR.

@phated

@sttk The tests are looking solid but I had some questions and code style changes inline.

index.js Outdated
@@ -20,6 +19,10 @@ var cliVersion = require('./package.json').version;
var getBlacklist = require('./lib/shared/getBlacklist');
var toConsole = require('./lib/shared/log/toConsole');
var loadConfigFiles = require('./lib/shared/config/loadfiles');

This comment has been minimized.

@phated

phated Jan 11, 2017

Member

rename to load-files please.

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

I got it.

index.js Outdated
}, handleArguments);
};
cli.launch(envOpts, function(env) {

This comment has been minimized.

@phated

phated Jan 11, 2017

Member

This should be a named function

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

I got it.

index.js Outdated
cli.launch(envOpts, function(env) {
var config;
try {
config = loadConfigFiles(env.configFiles['.gulp'], ['home', 'cwd']);

This comment has been minimized.

@phated

phated Jan 11, 2017

Member

Why does this have the chance to throw? Can it be rewritten to not throw?

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

This exception is caused by validation of config files. Does your comment mean that outputting error and exiting should be done in loadConfigFiles, or it is unnecessary to validate config files?

This comment has been minimized.

@phated

phated Jan 17, 2017

Member

Do we need to validate the config? Could we just ignore and log a warning or error if it's the wrong type?

I don't like the throw/try/catch pattern and we shouldn't be exiting.

This comment has been minimized.

@sttk

sttk Jan 21, 2017

Member

I got it. I'll make display errors but not exit.

index.js Outdated
}
mergeToCliFlags(opts, config, cliOptions);
mergeToEnvFlags(env, config, envOpts);

This comment has been minimized.

@phated

phated Jan 11, 2017

Member

Why do we pass envOpts in here?

This comment has been minimized.

@phated

phated Jan 11, 2017

Member

This should be non-mutating (return a new object).

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

Why do we pass envOpts in here?

To exclude env property names related to command-line arguments.

This should be non-mutating (return a new object).

I got it.

This comment has been minimized.

@phated

phated Jan 17, 2017

Member

To exclude env property names related to command-line arguments.

Can you explain this further?

index.js Outdated
exit(1);
}
mergeToCliFlags(opts, config, cliOptions);

This comment has been minimized.

@phated

phated Jan 11, 2017

Member

This should be non-mutating (return a new object).

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

I got it.

if (!envOpts.configPath && cfgFlags.gulpfile) {
envFlags.configPath = cfgFlags.gulpfile;
envFlags.configBase = path.dirname(cfgFlags.gulpfile);

This comment has been minimized.

@phated

phated Jan 11, 2017

Member

This should be creating a new object instead of mutating

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

I got it.

function loadConfigFiles(configFilesBase, keysInOrder) {
var config = {};
var configFilePaths = keysInOrder.map(function(key) {

This comment has been minimized.

@phated

phated Jan 11, 2017

Member

Does fined not do this for us automatically? It's okay if it doesn't but my mind is fuzzy on it.

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

Properties order in objects is not guaranteed, though Object.keys looks like returning in definition order actually. I think it is needed to specify the order explicitly.

This comment has been minimized.

@phated

phated Jan 17, 2017

Member

Sorry, I meant that fined crawls the file system and only gives us the config file that matches. Or are we using it differently?

This comment has been minimized.

@sttk

sttk Jan 21, 2017

Member

If config files are found in user home directory and project directory, both file paths are given by fined. In that case each config in both project dir should be given priority over each config in user home dir, I think.

Though this order might be determined by descriptions at Liftoff constructor, I want to specify it explicitly.

--
EDIT: erase the word "both"

@@ -0,0 +1,33 @@
'use strict';
var get = require('lodash.get');

This comment has been minimized.

@phated

phated Jan 11, 2017

Member

I'm going to be removing lodash. It's okay for us to leave it in for this implementation unless you want to find replacements

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

If it is good to use copy-props, I'll replace this to it.

This comment has been minimized.

@phated

phated Jan 17, 2017

Member

Are we using it in other places? I'm fine with using it since you will be able to fix anything needed.

return;
}
set(config, keyPath, spec.validate(value, filePath));

This comment has been minimized.

@phated

phated Jan 11, 2017

Member

What is validate doing in this scenario?

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

Validating each config value comming from config files, and convert file path (e.g. gulpfile) from relative path from .gulp.* to absolute path.

This comment has been minimized.

@phated

phated Jan 17, 2017

Member

I don't like the validate stuff. I think there's a better way to do this.

@@ -39,8 +38,8 @@ function execute(opts, env) {
}
if (opts.tasks) {
var tree = taskTree(gulpInst.tasks);
if (opts.description && isString(opts.description)) {
tree.label = opts.description;
if (config.description) {

This comment has been minimized.

@phated

phated Jan 11, 2017

Member

Will this always exist now?

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

About config object? It will be always exists.

This comment has been minimized.

@phated

phated Jan 17, 2017

Member

Is config.description always guaranteed to be undefined or a String?

This comment has been minimized.

@sttk

sttk Jan 21, 2017

Member

Yes. It will be checked and output errors if invalid.

This comment has been minimized.

@phated

phated Apr 19, 2017

Member

Put the isString check back in when the validation is removed.

@sttk

I'll modify what you pointed out.

index.js Outdated
@@ -20,6 +19,10 @@ var cliVersion = require('./package.json').version;
var getBlacklist = require('./lib/shared/getBlacklist');
var toConsole = require('./lib/shared/log/toConsole');
var loadConfigFiles = require('./lib/shared/config/loadfiles');

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

I got it.

index.js Outdated
}, handleArguments);
};
cli.launch(envOpts, function(env) {

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

I got it.

index.js Outdated
cli.launch(envOpts, function(env) {
var config;
try {
config = loadConfigFiles(env.configFiles['.gulp'], ['home', 'cwd']);

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

This exception is caused by validation of config files. Does your comment mean that outputting error and exiting should be done in loadConfigFiles, or it is unnecessary to validate config files?

index.js Outdated
exit(1);
}
mergeToCliFlags(opts, config, cliOptions);

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

I got it.

index.js Outdated
}
mergeToCliFlags(opts, config, cliOptions);
mergeToEnvFlags(env, config, envOpts);

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

Why do we pass envOpts in here?

To exclude env property names related to command-line arguments.

This should be non-mutating (return a new object).

I got it.

if (!envOpts.configPath && cfgFlags.gulpfile) {
envFlags.configPath = cfgFlags.gulpfile;
envFlags.configBase = path.dirname(cfgFlags.gulpfile);

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

I got it.

@@ -0,0 +1,33 @@
'use strict';
var get = require('lodash.get');

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

If it is good to use copy-props, I'll replace this to it.

function loadConfigFiles(configFilesBase, keysInOrder) {
var config = {};
var configFilePaths = keysInOrder.map(function(key) {

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

Properties order in objects is not guaranteed, though Object.keys looks like returning in definition order actually. I think it is needed to specify the order explicitly.

return;
}
set(config, keyPath, spec.validate(value, filePath));

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

Validating each config value comming from config files, and convert file path (e.g. gulpfile) from relative path from .gulp.* to absolute path.

@@ -39,8 +38,8 @@ function execute(opts, env) {
}
if (opts.tasks) {
var tree = taskTree(gulpInst.tasks);
if (opts.description && isString(opts.description)) {
tree.label = opts.description;
if (config.description) {

This comment has been minimized.

@sttk

sttk Jan 14, 2017

Member

About config object? It will be always exists.

@phated

This comment has been minimized.

Member

phated commented Jan 25, 2017

@sttk Don't mean to pester you but do you know when you'll have a chance to work on the changes?

@sttk

This comment has been minimized.

Member

sttk commented Jan 25, 2017

@phated I'll work on the changes this weekend. Why?

@phated

This comment has been minimized.

Member

phated commented Jan 25, 2017

No reason, I've just been really active on the project lately. I know how hard it is to get me to review, haha!

sttk
@sttk

This comment has been minimized.

Member

sttk commented Jan 28, 2017

Since I've modified matters pointed out by you, please review.
And list flags that we will be able to specify with config files, please.

@phated

This comment has been minimized.

Member

phated commented Apr 19, 2017

@sttk I'm terrible. Sorry! Looking at this now.

@phated

Sorry this took me so long to review. I promise to review quicker on your next updates.

index.js Outdated
@@ -3,14 +3,13 @@
var fs = require('fs');
var path = require('path');
var log = require('gulplog');
var fancyLog = require('fancy-log');

This comment has been minimized.

@phated

phated Apr 19, 2017

Member

I really don't like using fancy-log directly, it means that someone using --silent will still see the messages. Looking into re-binding the logger after we parse the config.

This comment has been minimized.

@phated

phated Apr 19, 2017

Member

toConsole can be updated to remove previous listeners if called a 2nd time. You can then use it at the beginning of this file and call it again after you normalize the config.

index.js Outdated
cli.on('require', function(name) {
log.info('Requiring external module', chalk.magenta(name));
fancyLog('Requiring external module', chalk.magenta(name));

This comment has been minimized.

@phated

phated Apr 19, 2017

Member

See above about fancy-log

index.js Outdated
});
cli.on('requireFail', function(name) {
log.error(chalk.red('Failed to load external module'), chalk.magenta(name));
fancyLog.error(

This comment has been minimized.

@phated

phated Apr 19, 2017

Member

See above about fancy-log

index.js Outdated
}
module.exports = run;
// The actual logic
function handleArguments(env) {
function configureAndInvoke(env) {

This comment has been minimized.

@phated

phated Apr 19, 2017

Member

Is this just split out due to the linting complaining about number of statements? I'd be happier increasing the limit and putting it inside handleArguments.

'use strict';
var path = require('path');
var logger = require('fancy-log');

This comment has been minimized.

@phated

phated Apr 19, 2017

Member

See above about fancy-log

@@ -49,8 +48,8 @@ function execute(opts, env) {
}
if (opts.tasks) {
tree = gulpInst.tree({ deep: true });
if (opts.description && isString(opts.description)) {
tree.label = opts.description;
if (config.description) {

This comment has been minimized.

@phated

phated Apr 19, 2017

Member

Put the isString check back in when the validation is removed.

@@ -49,8 +48,8 @@ function execute(opts, env) {
}
if (opts.tasks) {
tree = gulpInst.tree({ deep: true });
if (opts.description && isString(opts.description)) {
tree.label = opts.description;
if (config.description) {

This comment has been minimized.

@phated

phated Apr 19, 2017

Member

Put the isString check back in when the validation is removed.

@@ -59,8 +58,8 @@ function execute(opts, env) {
}
if (opts.tasksJson) {
tree = gulpInst.tree({ deep: true });
if (opts.description && isString(opts.description)) {
tree.label = opts.description;
if (config.description) {

This comment has been minimized.

@phated

phated Apr 19, 2017

Member

Put the isString check back in when the validation is removed.

@@ -59,8 +58,8 @@ function execute(opts, env) {
}
if (opts.tasksJson) {
tree = gulpInst.tree({ deep: true });
if (opts.description && isString(opts.description)) {
tree.label = opts.description;
if (config.description) {

This comment has been minimized.

@phated

phated Apr 19, 2017

Member

Put the isString check back in when the validation is removed.

"cover": "nyc --reporter=lcov --reporter=text-summary npm test",
"coveralls": "nyc --reporter=text-lcov npm test | coveralls",
"changelog": "github-changes -o gulpjs -r gulp-cli -b master -f ./CHANGELOG.md --order-semver --use-commit-body"
},
"dependencies": {
"archy": "^1.0.0",
"camelcase": "^3.0.0",

This comment has been minimized.

@phated

phated Apr 19, 2017

Member

This can be removed when excludeCliArgs is removed.

@sttk

This comment has been minimized.

Member

sttk commented Apr 20, 2017

@phated Thanks for your reviewing. I'll address what you pointed out this week end.

@sttk

This comment has been minimized.

Member

sttk commented Apr 22, 2017

@phated I've modified.

--
EDIT:
Sorry. I forgot to remove camelcase from dependencies. I've done.

sttk
copyProps(require(filePath), config, function(value, name) {
if (name === 'flags.gulpfile') {
return path.resolve(path.dirname(filePath), value);

This comment has been minimized.

@phated

phated Apr 22, 2017

Member

Oh interesting, this is making the path in the config file relative its location.

function loadConfigFiles(configFiles, configFileOrder) {
var config = {};
configFileOrder.forEach(function(key) {

This comment has been minimized.

@phated

phated Apr 22, 2017

Member

Named function (I can make this change during merge).

return;
}
copyProps(require(filePath), config, function(value, name) {

This comment has been minimized.

@phated

phated Apr 22, 2017

Member

Named function (I can make this change during merge).

@@ -14,6 +14,9 @@ var levels = [
];
function toConsole(log, opts) {
// Remove previous listeners to enable to call this twice.
log.removeAllListeners();

This comment has been minimized.

@phated

phated Apr 22, 2017

Member

I think this should be more specific to avoid removing other listeners someone else might have attached (we are using a global, after all). I can change this in the merge.

This comment has been minimized.

@sttk

sttk Apr 23, 2017

Member

Oh, you are exactly right. Thanks.

@phated

phated approved these changes Apr 22, 2017

@phated

This comment has been minimized.

Member

phated commented Apr 22, 2017

@sttk amazing work, as always. I'll get this merged and make a couple changes I commented about.

@phated phated merged commit 2732c66 into gulpjs:master Apr 22, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.6%) to 92.268%
Details

phated added a commit that referenced this pull request Apr 22, 2017

@sttk

This comment has been minimized.

Member

sttk commented Apr 23, 2017

@phated Thank you for merging!

@sttk sttk deleted the sttk:restrict_config_props branch Apr 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment