From f43433db4984120869d2500ecbcbeab16bcbee35 Mon Sep 17 00:00:00 2001 From: Ade Attwood Date: Wed, 12 May 2021 20:36:03 +0100 Subject: [PATCH] feat: add subject-exclamation-mark rule to improve error messages 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 | 84 +++++++++++++++++++ @commitlint/config-conventional/index.test.js | 34 ++++---- @commitlint/rules/src/index.ts | 2 + .../src/subject-exclamation-mark.test.ts | 52 ++++++++++++ .../rules/src/subject-exclamation-mark.ts | 21 +++++ docs/reference-rules.md | 5 ++ 8 files changed, 197 insertions(+), 16 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..31a8e0bfd7 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://docs.google.com/document/d/1QrDFcIiPjSLDn3EL15IJygNPiHORgU1_OOAqWjiDU5Y). 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](https://docs.google.com/document/d/1QrDFcIiPjSLDn3EL15IJygNPiHORgU1_OOAqWjiDU5Y) +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..56692a67f9 --- /dev/null +++ b/@commitlint/config-angular/index.test.js @@ -0,0 +1,84 @@ +import lint from '@commitlint/lint'; +import {rules, parserPreset} from '.'; + +export const testCommit = async ({message, valid, error, warning}) => { + const m = message.replace(/^\s+/, '').trim(); + test(m.split('\n')[0], async () => { + const parserOpts = parserPreset.parserOpts; + const result = await lint(m, rules, {parserOpts}); + + expect(result.valid).toBe(valid); + + if (result.errors.length > 1) { + console.log(errors); + fail( + 'Commit test should only have one error message to validate against' + ); + } else if (result.errors.length) { + expect(result.errors[0].message).toBe(error); + } + + if (result.warnings.length > 1) { + fail( + 'Commit test should only have one warning message to validate against' + ); + } else if (result.warnings.length) { + expect(result.warnings[0].message).toBe(warning); + } + }); +}; + +testCommit({ + message: `test: a valid angular commit`, + valid: true, +}); + +testCommit({ + message: `test(scope): a valid angular commit with a scope`, + valid: true, +}); + +testCommit({ + message: ` + test(scope): a valid angular commit with a scope + + Some content in the body + `, + valid: true, +}); + +testCommit({ + message: ` + test(scope): a valid angular commit with a scope + Some content in the body + `, + valid: true, + warning: 'body must have leading blank line', +}); + +testCommit({ + message: `no: no is not not an invalid commit type`, + valid: false, + error: + 'type must be one of [build, ci, docs, feat, fix, perf, refactor, revert, style, test]', +}); + +testCommit({ + message: `test: that its an error when there is ia realllllllllllllllllllllly long header`, + valid: false, + error: 'header must not be longer than 72 characters, current length is 79', +}); + +testCommit({ + message: `test!: with a breaking change in the type`, + valid: false, + error: + 'subject must not have an exclamation mark in the subject to identify a breaking change', +}); + +testCommit({ + message: `test(scope)!: with a breaking change in the type with scope`, + valid: false, + error: + 'subject must not have an exclamation mark in the subject to identify a breaking change', +}); 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 8053f721f3..def0d1f3d6 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 {typeCase} from './type-case'; import {typeEmpty} from './type-empty'; import {typeEnum} from './type-enum'; @@ -61,6 +62,7 @@ export default { 'subject-full-stop': subjectFullStop, 'subject-max-length': subjectMaxLength, 'subject-min-length': subjectMinLength, + 'subject-exclamation-mark': subjectExclamationMark, 'type-case': typeCase, 'type-empty': typeEmpty, 'type-enum': typeEnum, 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..aa1542aa57 --- /dev/null +++ b/@commitlint/rules/src/subject-exclamation-mark.test.ts @@ -0,0 +1,52 @@ +import parse from '@commitlint/parse'; +import {subjectExclamationMark} from './subject-exclamation-mark'; + +const {parserOpts} = require('@commitlint/config-angular'); + +const messages = { + empty: 'test:\n', + with: `test!: subject\n`, + without: `test: subject\n`, +}; + +const parsed = { + empty: parse(messages.empty, undefined, parserOpts), + with: parse(messages.with, undefined, parserOpts), + without: parse(messages.without, undefined, parserOpts), +}; + +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 a3a0385dd0..2d69a4bd99 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