Feature: predefined scope options#126
Feature: predefined scope options#126ThisIsMissEm wants to merge 5 commits intocommitizen:masterfrom
Conversation
…rompter This allows us to use methods from inquirer, such as the Separator class, before actually calling prompter()
Previously configuration was tested by mocking our own code, as well as code that we don't own (other modules), this was because configuration took place at require-time of the cz-conventional-commit module, rather than at execution-time. By moving configuration evaluation to execution-time we can better test the logic involved in loading and evaluating configuration. The removal of the dependency on cosmiconfig comes because we no longer need to mock this module which is a 2nd degree dependency: it's used by commitlint, which we can just mock directly. The previous mocking was needed to override the max-header-length option in a really round-about manner where all tests effectively depended on commitlint, even though that wasn't what the functionality actually was. This changeset all means that we can test loading and evaluating configuration in isolation, without involving the engine.
Allow users to configure a predefined list of scopes that can be selected from, with the option to type their own scope if it's not on the list. When passing CZ_SCOPE, we attempt to pre-select the scope from the list of scopes, or auto-select "something else" which allows the user to confirm the CZ_SCOPE value.
Previously specifying a custom max header width wasn't actually tested for, only the configuration loading was tested. This ensures that we have coverage on this functionality.
| // By default, we'll de-indent your commit | ||
| // template and will keep empty lines. | ||
| prompter: function(cz, commit) { | ||
| prompter: function(commit) { |
There was a problem hiding this comment.
We may be able to simplify the engine even more by just having it directly run cz.prompt / inquirer.prompt, but doing such a change right now would result in the changes due to predefined scopes being obscured. THis future change would look like:
module.exports = function(options, inquirer, commit) {
// ...
inquirer.prompt().then(...)
}
Without returning a "prompter" method here.
There was a problem hiding this comment.
Essentially I'm trying to make this easier to review by avoiding changes that would make additional lines of code look changed when they've not actually changed, other than an indentation level change.
| // the clearer separation of concerns now present. | ||
| // | ||
| // For details of the prior test, see: https://github.com/commitizen/cz-conventional-changelog/blob/e7bd5462966d00acb03aca394836b5427513681c/engine.test.js#L391-L406 | ||
| it('should correctly load configuration from commitizen'); |
There was a problem hiding this comment.
I'm not sure if we really need to test require('commitizen').configLoader.load(), I have also opened up commitizen/cz-cli#757 which would remove the dependency we have on commitizen, such that we'd simply say "here's the configuration commitizen has, here's own custom stuff with the overlay from committing"
I think it'd be better to make a backwards-compatible change in cz-cli than write a reasonably nasty test case that mocks 2nd-degree dependencies.
|
@ThisIsMissEm Any news? |
@btd1337 usually if there's news on something, then the person would say. |
|
Nice! I would like to use this feature! |
|
Looking at this feature — someone merge it please! |
4 participants
This is an alternative implementation to #118 for the feature requested in #117 — the functionality is such that:
Via configuration or via environment variables the user can configure a set of predefined scopes to select from when doing
git-czto create their commit message.This is different from #118, in that we always allow the user to specify their own scope if they don't like the predefined options, making this change less strict that #118 which enforces only using the predefined scopes. My idea with this is that we're nudging the user towards selecting a predefined scope, without preventing them from using something else.
I've also taken the liberty to refactor the configuration loading to improve the testability of that code, previously a lot of module mocking was required due to the configuration being evaluated at require-time of
cz-conventional-commitrather than at execution time. By moving configuration evaluation to execution-time we can now test the logic inindex.jswithout heavily mocking our own code and that of others and their dependencies.I still need to finish writing tests for the scope choices, but the existing tests still pass with only little modification. If you're interested in even more detail around the changes, please take a look at the individual commits, as they each are complete and test-runnable.