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

@@ -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

@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],

@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([]);
+});

@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"

@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,31 +113,29 @@ 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]);
 });
 
 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,57 +145,57 @@ 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]);
 });
 
 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) => {

@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,

@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);
+});

@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',
+		]),
+	];
+};

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