Skip to content
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 that are merged into opts from .gulp.* files #92

Closed
phated opened this issue Aug 23, 2016 · 6 comments
Closed
Milestone

Comments

@phated
Copy link
Member

phated commented Aug 23, 2016

@sttk I created this issue based on #71 (comment)

I'd like to keep the top-level properties restricted so they don't conflict in the future. e.g. a --description flag that does something different than the configuration description property.

Do you agree?

@phated phated added this to the 1.3.0 milestone Aug 23, 2016
@sttk
Copy link
Contributor

sttk commented Aug 24, 2016

@phated Sorry for my thoughtless. Certainly cli flags should be distinguished from config properties. I agree.

@sttk
Copy link
Contributor

sttk commented Aug 25, 2016

I'd like to change this to use the specification object like lib/shared/cliOptions.js for cli flags. The implementation image is as follows:

var configSpecs = require('lib/shared/configs/')
var checkConfig = require('lib/shared/configs/checkConfig')
  ...
  // The target merged with configs is an empty object.
  var config = {};

  var configFilePaths = ['home', 'cwd'].map(function(prop) {
    return env.configFiles['.gulp'][prop];
  });
  configFilePaths.filter(isString).forEach(function(filePath) {
    // The target object is merged with results of checks with the spec. object.
    merge(config, checkConfig(require(filePath), configSpecs));
  });
  ...

  // The target is marged with flags in `opts` specified in the spec object.
  merge(config, checkConfig(opts, lodash.filter(configSpecs, { cliFlag: true })));

And I think it's good to pass the config object to versioned index.js (lib/versioned/*/index.js) instead of opts in the future, but it will now be passed the config as another argument temporarily.

  require(path.join(__dirname, '/lib/versioned/', range, '/'))(opts, env, config);

What do you think about this?

@phated
Copy link
Member Author

phated commented Aug 25, 2016

@sttk I think it is fine to continue to merge with opts but I don't think .gulp.* and opts should have the same shape. I'm fine with adding helper methods. I think we need to do something similar to lodash.pick but for deep properties, like pick(require(filePath), ['description', 'flags.continue', 'flags.silent', 'flags.logLevel', ...]). Thoughts?

@sttk
Copy link
Contributor

sttk commented Aug 26, 2016

@phated

I think we need to do something similar to lodash.pick but for deep properties, ...

Yes, we would need something like that. And there are lodash.get/set to access nested properies of an object. We would be able to use them in the helper methods.

@phated
Copy link
Member Author

phated commented Aug 26, 2016

It would be easiest to have an array of paths but I wonder if a config object that we merge into is better.

sttk pushed a commit to sttk/gulp-cli that referenced this issue Sep 4, 2016
sttk pushed a commit to sttk/gulp-cli that referenced this issue Sep 4, 2016
@sttk
Copy link
Contributor

sttk commented Sep 4, 2016

I've implemented for this issue and sended it to PR #94.

sttk pushed a commit to sttk/gulp-cli that referenced this issue Oct 10, 2016
sttk pushed a commit to sttk/gulp-cli that referenced this issue Oct 10, 2016
sttk pushed a commit to sttk/gulp-cli that referenced this issue Oct 11, 2016
@phated phated closed this as completed in 2732c66 Apr 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants