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

Feature: predefined scope options #126

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ThisIsMissEm
Copy link

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-cz to 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-commit rather than at execution time. By moving configuration evaluation to execution-time we can now test the logic in index.js without 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.

…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.
//
// The commit callback should be executed when
// you're ready to send back a commit template
// to git.
//
// By default, we'll de-indent your commit
// template and will keep empty lines.
prompter: function(cz, commit) {
prompter: function(commit) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@btd1337
Copy link

btd1337 commented Jan 25, 2021

@ThisIsMissEm Any news?

@ThisIsMissEm
Copy link
Author

Emelia Smith Any news?

@btd1337 usually if there's news on something, then the person would say.

@nelson6e65
Copy link

Nice! I would like to use this feature!

@dvakatsiienko
Copy link

Looking at this feature — someone merge it please!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants