From be701bdb1de4e667b7a872767244285c4fa4fda4 Mon Sep 17 00:00:00 2001 From: Ade Attwood Date: Mon, 24 May 2021 07:51:32 +0100 Subject: [PATCH] feat: add subject-exclamation-mark rule to improve error messages (#2593) When using the conventional commit feature of an `!` in the commit subject the angular config get really confused and gives some error messages that do not relate to the issue due to the message failing at the parse stage. This overrides the angular parser preset to add in the exclamation mark then uses the new rule to give a better error message. The result is with the message "fix!: the fix" previously the error message would be "subject may not be empty" now the error message is "subject must not have an exclamation mark in the subject to identify a breaking change". This message it more descriptive and will give the user info they need to resolve the issue. This also updates the docs to highlight the difference in angular and conventional configs, and point them in the direction of the conventional config if they want to use the `!` in the commit messages --- @commitlint/config-angular/README.md | 13 ++- @commitlint/config-angular/index.js | 2 + @commitlint/config-angular/index.test.js | 104 ++++++++++++++++++ @commitlint/config-angular/package.json | 3 +- @commitlint/config-conventional/index.test.js | 34 +++--- @commitlint/rules/src/index.ts | 2 + .../src/subject-exclamation-mark.test.ts | 57 ++++++++++ .../rules/src/subject-exclamation-mark.ts | 21 ++++ docs/reference-rules.md | 5 + 9 files changed, 224 insertions(+), 17 deletions(-) create mode 100644 @commitlint/config-angular/index.test.js create mode 100644 @commitlint/rules/src/subject-exclamation-mark.test.ts create mode 100644 @commitlint/rules/src/subject-exclamation-mark.ts diff --git a/@commitlint/config-angular/README.md b/@commitlint/config-angular/README.md index 76ac78b3a7..603414339d 100644 --- a/@commitlint/config-angular/README.md +++ b/@commitlint/config-angular/README.md @@ -2,7 +2,7 @@ # @commitlint/config-angular -Shareable `commitlint` config enforcing the [Angular commit convention](https://github.com/angular/angular/blob/master/CONTRIBUTING.md#-commit-message-guidelines). +Shareable `commitlint` config enforcing the [Angular commit convention](https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit). Use with [@commitlint/cli](../cli) and [@commitlint/prompt-cli](../prompt-cli). ## Getting started @@ -122,6 +122,17 @@ echo "fix: some message." # fails echo "fix: some message" # passes ``` +#### subject-exclamation-mark + +- **condition**: `subject` must not have a `!` before the `:` marker +- **rule**: `never` + +The [angular commit +convention](hhttps://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit) +dose not use a `!` to define a breaking change in the commit subject. If you +want to use this feature please consider using the [conventional commit +config](https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional#commitlintconfig-conventional). + #### header-max-length - **condition**: `header` has `value` or less characters diff --git a/@commitlint/config-angular/index.js b/@commitlint/config-angular/index.js index 57c82621c7..12e2a00aa6 100644 --- a/@commitlint/config-angular/index.js +++ b/@commitlint/config-angular/index.js @@ -1,7 +1,9 @@ const typeEnum = require('@commitlint/config-angular-type-enum'); module.exports = { + parserPreset: {parserOpts: {headerPattern: /^(\w*)(?:\((.*)\))?!?: (.*)$/}}, rules: { + 'subject-exclamation-mark': [2, 'never'], 'body-leading-blank': [1, 'always'], 'footer-leading-blank': [1, 'always'], 'header-max-length': [2, 'always', 72], diff --git a/@commitlint/config-angular/index.test.js b/@commitlint/config-angular/index.test.js new file mode 100644 index 0000000000..6b5919d1fb --- /dev/null +++ b/@commitlint/config-angular/index.test.js @@ -0,0 +1,104 @@ +import lint from '@commitlint/lint'; +import {rules, parserPreset} from '.'; + +const lintMessage = async (message) => { + const parserOpts = parserPreset.parserOpts; + const m = message.replace(/^\s+/, '').trim(); + const result = await lint(m, rules, {parserOpts}); + + if (result.errors.length > 1) { + throw new Error( + 'Commit test should only have one error message to validate against' + ); + } + + if (result.warnings.length > 1) { + throw new Error( + 'Commit test should only have one warning message to validate against' + ); + } + + return result; +}; + +test('a valid commit message', async () => { + const result = await lintMessage('test: a valid angular commit'); + expect(result.valid).toBe(true); + expect(result.errors).toStrictEqual([]); + expect(result.warnings).toStrictEqual([]); +}); + +test('a valid message with a scope', async () => { + const result = await lintMessage( + 'test(scope): a valid angular commit with a scope' + ); + expect(result.valid).toBe(true); + expect(result.errors).toStrictEqual([]); + expect(result.warnings).toStrictEqual([]); +}); + +test('a valid multi line commit', async () => { + const result = await lintMessage( + `test(scope): a valid angular commit with a scope + + Some content in the body` + ); + expect(result.valid).toBe(true); + expect(result.errors).toStrictEqual([]); + expect(result.warnings).toStrictEqual([]); +}); + +test('a leading blank line after header', async () => { + const result = await lintMessage( + `test(scope): a valid angular commit with a scope + Some content in the body` + ); + + expect(result.valid).toBe(true); + expect(result.errors).toStrictEqual([]); + expect(result.warnings[0].message).toBe('body must have leading blank line'); +}); + +test('an invalid scope', async () => { + const result = await lintMessage(`no: no is not not an invalid commit type`); + + expect(result.valid).toBe(false); + expect(result.errors[0].message).toBe( + 'type must be one of [build, ci, docs, feat, fix, perf, refactor, revert, style, test]' + ); + expect(result.warnings).toStrictEqual([]); +}); + +test('a long header', async () => { + const result = await lintMessage( + `test: that its an error when there is ia realllllllllllllllllllllly long header` + ); + + expect(result.valid).toBe(false); + expect(result.errors[0].message).toBe( + 'header must not be longer than 72 characters, current length is 79' + ); + expect(result.warnings).toStrictEqual([]); +}); + +test('message header with ! in it', async () => { + const result = await lintMessage(`test!: with a breaking change in the type`); + + expect(result.valid).toBe(false); + expect(result.errors[0].message).toBe( + 'subject must not have an exclamation mark in the subject to identify a breaking change' + ); + expect(result.warnings).toStrictEqual([]); +}); + +test('message header with ! in it and a scope', async () => { + const result = await lintMessage( + `test(scope)!: with a breaking change in the type` + ); + + expect(result.valid).toBe(false); + expect(result.errors[0].message).toBe( + 'subject must not have an exclamation mark in the subject to identify a breaking change' + ); + expect(result.warnings).toStrictEqual([]); +}); diff --git a/@commitlint/config-angular/package.json b/@commitlint/config-angular/package.json index ba4b06eef8..6f06eb616d 100644 --- a/@commitlint/config-angular/package.json +++ b/@commitlint/config-angular/package.json @@ -29,7 +29,8 @@ "node": ">=v12" }, "devDependencies": { - "@commitlint/utils": "^12.1.4" + "@commitlint/utils": "^12.1.4", + "@commitlint/lint": "^12.1.4" }, "dependencies": { "@commitlint/config-angular-type-enum": "^12.1.4" diff --git a/@commitlint/config-conventional/index.test.js b/@commitlint/config-conventional/index.test.js index 570c4b9ace..f3023b6819 100644 --- a/@commitlint/config-conventional/index.test.js +++ b/@commitlint/config-conventional/index.test.js @@ -1,5 +1,10 @@ import lint from '@commitlint/lint'; -import {rules} from '.'; +import {rules, parserPreset} from '.'; + +const commitLint = async (message) => { + const preset = await require(parserPreset)(); + return lint(message, rules, {...preset}); +}; const messages = { invalidTypeEnum: 'foo: some message', @@ -28,6 +33,7 @@ const messages = { 'fix(scope): some Message', 'fix(scope): some message\n\nBREAKING CHANGE: it will be significant!', 'fix(scope): some message\n\nbody', + 'fix(scope)!: some message\n\nbody', ], }; @@ -107,21 +113,21 @@ const warnings = { }; test('type-enum', async () => { - const result = await lint(messages.invalidTypeEnum, rules); + const result = await commitLint(messages.invalidTypeEnum); expect(result.valid).toBe(false); expect(result.errors).toEqual([errors.typeEnum]); }); test('type-case', async () => { - const result = await lint(messages.invalidTypeCase, rules); + const result = await commitLint(messages.invalidTypeCase); expect(result.valid).toBe(false); expect(result.errors).toEqual([errors.typeCase, errors.typeEnum]); }); test('type-empty', async () => { - const result = await lint(messages.invalidTypeEmpty, rules); + const result = await commitLint(messages.invalidTypeEmpty); expect(result.valid).toBe(false); expect(result.errors).toEqual([errors.typeEmpty]); @@ -129,9 +135,7 @@ test('type-empty', async () => { test('subject-case', async () => { const invalidInputs = await Promise.all( - messages.invalidSubjectCases.map((invalidInput) => - lint(invalidInput, rules) - ) + messages.invalidSubjectCases.map((invalidInput) => commitLint(invalidInput)) ); invalidInputs.forEach((result) => { @@ -141,49 +145,49 @@ test('subject-case', async () => { }); test('subject-empty', async () => { - const result = await lint(messages.invalidSubjectEmpty, rules); + const result = await commitLint(messages.invalidSubjectEmpty); expect(result.valid).toBe(false); expect(result.errors).toEqual([errors.subjectEmpty, errors.typeEmpty]); }); test('subject-full-stop', async () => { - const result = await lint(messages.invalidSubjectFullStop, rules); + const result = await commitLint(messages.invalidSubjectFullStop); expect(result.valid).toBe(false); expect(result.errors).toEqual([errors.subjectFullStop]); }); test('header-max-length', async () => { - const result = await lint(messages.invalidHeaderMaxLength, rules); + const result = await commitLint(messages.invalidHeaderMaxLength); expect(result.valid).toBe(false); expect(result.errors).toEqual([errors.headerMaxLength]); }); test('footer-leading-blank', async () => { - const result = await lint(messages.warningFooterLeadingBlank, rules); + const result = await commitLint(messages.warningFooterLeadingBlank, rules); expect(result.valid).toBe(true); expect(result.warnings).toEqual([warnings.footerLeadingBlank]); }); test('footer-max-line-length', async () => { - const result = await lint(messages.invalidFooterMaxLineLength, rules); + const result = await commitLint(messages.invalidFooterMaxLineLength); expect(result.valid).toBe(false); expect(result.errors).toEqual([errors.footerMaxLineLength]); }); test('body-leading-blank', async () => { - const result = await lint(messages.warningBodyLeadingBlank, rules); + const result = await commitLint(messages.warningBodyLeadingBlank); expect(result.valid).toBe(true); expect(result.warnings).toEqual([warnings.bodyLeadingBlank]); }); test('body-max-line-length', async () => { - const result = await lint(messages.invalidBodyMaxLineLength, rules); + const result = await commitLint(messages.invalidBodyMaxLineLength); expect(result.valid).toBe(false); expect(result.errors).toEqual([errors.bodyMaxLineLength]); @@ -191,7 +195,7 @@ test('body-max-line-length', async () => { test('valid messages', async () => { const validInputs = await Promise.all( - messages.validMessages.map((input) => lint(input, rules)) + messages.validMessages.map((input) => commitLint(input)) ); validInputs.forEach((result) => { diff --git a/@commitlint/rules/src/index.ts b/@commitlint/rules/src/index.ts index 60e3239453..af8ea6a544 100644 --- a/@commitlint/rules/src/index.ts +++ b/@commitlint/rules/src/index.ts @@ -26,6 +26,7 @@ import {subjectEmpty} from './subject-empty'; import {subjectFullStop} from './subject-full-stop'; import {subjectMaxLength} from './subject-max-length'; import {subjectMinLength} from './subject-min-length'; +import {subjectExclamationMark} from './subject-exclamation-mark'; import {trailerExists} from './trailer-exists'; import {typeCase} from './type-case'; import {typeEmpty} from './type-empty'; @@ -62,6 +63,7 @@ export default { 'subject-full-stop': subjectFullStop, 'subject-max-length': subjectMaxLength, 'subject-min-length': subjectMinLength, + 'subject-exclamation-mark': subjectExclamationMark, 'trailer-exists': trailerExists, 'type-case': typeCase, 'type-empty': typeEmpty, diff --git a/@commitlint/rules/src/subject-exclamation-mark.test.ts b/@commitlint/rules/src/subject-exclamation-mark.test.ts new file mode 100644 index 0000000000..1b36eb8e3b --- /dev/null +++ b/@commitlint/rules/src/subject-exclamation-mark.test.ts @@ -0,0 +1,57 @@ +import parse from '@commitlint/parse'; +import {subjectExclamationMark} from './subject-exclamation-mark'; + +const preset = require('conventional-changelog-angular'); + +const parseMessage = async (str: string) => { + const {parserOpts} = await preset; + return parse(str, undefined, parserOpts); +}; + +const messages = { + empty: 'test:\n', + with: `test!: subject\n`, + without: `test: subject\n`, +}; + +const parsed = { + empty: parseMessage(messages.empty), + with: parseMessage(messages.with), + without: parseMessage(messages.without), +}; + +test('empty against "always" should fail', async () => { + const [actual] = subjectExclamationMark(await parsed.empty, 'always'); + const expected = false; + expect(actual).toEqual(expected); +}); + +test('empty against "never" should succeed', async () => { + const [actual] = subjectExclamationMark(await parsed.empty, 'never'); + const expected = true; + expect(actual).toEqual(expected); +}); + +test('with against "always" should succeed', async () => { + const [actual] = subjectExclamationMark(await parsed.with, 'always'); + const expected = true; + expect(actual).toEqual(expected); +}); + +test('with against "never" should fail', async () => { + const [actual] = subjectExclamationMark(await parsed.with, 'never'); + const expected = false; + expect(actual).toEqual(expected); +}); + +test('without against "always" should fail', async () => { + const [actual] = subjectExclamationMark(await parsed.without, 'always'); + const expected = false; + expect(actual).toEqual(expected); +}); + +test('without against "never" should succeed', async () => { + const [actual] = subjectExclamationMark(await parsed.without, 'never'); + const expected = true; + expect(actual).toEqual(expected); +}); diff --git a/@commitlint/rules/src/subject-exclamation-mark.ts b/@commitlint/rules/src/subject-exclamation-mark.ts new file mode 100644 index 0000000000..11cc29b590 --- /dev/null +++ b/@commitlint/rules/src/subject-exclamation-mark.ts @@ -0,0 +1,21 @@ +import message from '@commitlint/message'; +import {SyncRule} from '@commitlint/types'; + +export const subjectExclamationMark: SyncRule = (parsed, when = 'always') => { + const input = parsed.header; + if (!input) { + return [true, '']; + } + + const negated = when === 'never'; + const hasExclamationMark = /!:/.test(input); + + return [ + negated ? !hasExclamationMark : hasExclamationMark, + message([ + 'subject', + negated ? 'must not' : 'must', + 'have an exclamation mark in the subject to identify a breaking change', + ]), + ]; +}; diff --git a/docs/reference-rules.md b/docs/reference-rules.md index c4042dbc0b..9fbd605e01 100644 --- a/docs/reference-rules.md +++ b/docs/reference-rules.md @@ -336,6 +336,11 @@ Infinity 0 ``` +#### subject-exclamation-mark + +- **condition**: `subject` has exclamation before the `:` marker +- **rule**: `never` + #### type-enum - **condition**: `type` is found in value