shipshapecode / ember-cli-release

Ember CLI addon for versioned release management
MIT License
90 stars 18 forks source link

Fix bug where config file was not being loaded. #10

Closed lukemelia closed 9 years ago

lukemelia commented 9 years ago

An incorrect file existence check resulted in the config file never actually being loaded.

In testing this functionality, we also realized that the goal of having explicit CLI options take precedence over the config file is not currently possible. We can’t distinguish between explicitly specified CLI options and their default values because ember-cli pre-applies the default values and passes us the merged result.

In light of this and in order to provide a consistent experience, this commit updates the behavior and documentation to make options specified via the config file take precedence over explicit CLI options and their default values.

lukemelia commented 9 years ago

In particular, if an end-user was to specify a CLI option that matches a default value (e.g. --strategy=semver), its impossible to distinguish this from leaving that option off altogether.

rwjblue commented 9 years ago

I think that we could preserve the original intent, by removing the default values from the availableOptions hash and just keep track of defaults in a separate hash (instead of this).

We could keep the default option values on the object in a simple format to make it easier to do the merging:

defaultOptionValues: {
  local: true,
  remote: 'origin',
  message: 'Released %@',
  // .....
}

Then this line would become something like:

options = merge(options, cliOptions, this.defaultOptions);

Thoughts?

lukemelia commented 9 years ago

@rwjblue We considered this, but we would lose the benefit of ember-cli tooling showing you the defaults. Seems like something that would be better to "fix" in ember-cli, by somehow providing commands with the information about what options were set explicitly, and which from defaults.

rwjblue commented 9 years ago

Well, we can configure how ember-cli prints the help for our command (see here), but inlining all that code here would be pretty annoying.

Ideally (for our case at least) would be if the option processing was its own function (which itself used smaller functions like this.getOptionDefault(option) or this.getOptionName ) to allow simpler customization of just the option output.

rwjblue commented 9 years ago

Just chatted with @lukemelia in Slack, and suggested using a getter for availableOptions so that we can get the behavior that we originally wanted.

We can use the config/release.js files values for each option as the actual default and then still get the proper precedence order (higher to lower: command line, config file, default). We can indicate that the default value came from config/release.js in the options description.

I'd really like to preserve the order of precedence if possible, and if the above suggestion works that would be awesome...

chrislopresto commented 9 years ago

We should close this in favor of https://github.com/lytics/ember-cli-release/pull/12, which subsumes it.