From a884d91a69b53d352c25ec439874a62f05985909 Mon Sep 17 00:00:00 2001 From: Marko Ristin Date: Wed, 9 Sep 2020 14:37:42 +0200 Subject: [PATCH] Instruct how to remedy the issues (#68) In many teams, the wider team is not aware how the GitHub action has been set up and merely sees the messages in their CI reports. These users are ultimately lost and need further guidance on how to fix the errors in their commit messages. In particular, sparse error messages with no hints how to remedy the commit messages are almost useless leaving the users utterly frustrated. This change introduces very verbose error messages which include the hints what can be done about the message. While the messages are now quite long and seem cluttered, they are hopefully much more useful to the inexperienced users. --- .eslintrc.json | 2 +- dist/index.js | 289 ++++++++++++------ .../OpinionatedCommitMessage.Tests.ps1 | 90 +++++- local/powershell/OpinionatedCommitMessage.ps1 | 113 +++++-- .../OpinionatedCommitMessage.ps1.template | 112 +++++-- src/__tests__/input.test.ts | 31 ++ src/__tests__/inspection.test.ts | 192 +++++++++--- src/__tests__/main.test.ts | 42 +-- src/input.ts | 109 +++++++ src/inspection.ts | 189 ++++++++---- src/mainImpl.ts | 72 ++--- src/mostFrequentEnglishVerbs.ts | 3 +- 12 files changed, 909 insertions(+), 335 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index 5e51e36..ea70cb9 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -12,7 +12,7 @@ "import/no-namespace": "off", "no-unused-vars": "off", "@typescript-eslint/no-unused-vars": "error", - "@typescript-eslint/explicit-member-accessibility": ["error", {"accessibility": "no-public"}], + "@typescript-eslint/explicit-member-accessibility": "off", "@typescript-eslint/no-require-imports": "error", "@typescript-eslint/array-type": "error", "@typescript-eslint/await-thenable": "error", diff --git a/dist/index.js b/dist/index.js index 4935d62..92427be 100644 --- a/dist/index.js +++ b/dist/index.js @@ -577,11 +577,72 @@ function splitSubjectBody(lines) { result.subjectBody = { subject: lines[0], bodyLines: lines.slice(2) }; return result; } -const capitalizedWordRe = new RegExp('^([A-Z][a-z]*)[^a-zA-Z]'); +const allLettersRe = new RegExp('^[a-zA-Z][a-zA-Z-]+$'); +const firstWordBeforeSpaceRe = new RegExp('^([a-zA-Z][a-zA-Z-]+)\\s'); const suffixHashCodeRe = new RegExp('\\s?\\(\\s*#[a-zA-Z_0-9]+\\s*\\)$'); -function checkSubject(subject, additionalVerbs) { +function extractFirstWord(text) { + if (text.length === 0) { + return null; + } + if (text.match(allLettersRe)) { + return text; + } + else { + const match = firstWordBeforeSpaceRe.exec(text); + if (!match) { + return null; + } + return match[1]; + } +} +function capitalize(word) { + if (word.length === 0) { + return ''; + } + else if (word.length === 1) { + return word.toUpperCase(); + } + else { + return word[0].toUpperCase() + word.slice(1).toLowerCase(); + } +} +function errorMessageOnNonVerb(firstWord, inputs) { + const parts = [ + 'The subject must start with a verb in imperative mood, ' + + `but it started with: ${JSON.stringify(firstWord)}. ` + + 'Whether the word is in imperative mood is determined by ' + + 'whitelisting. The general whitelist is available at ' + + 'https://github.com/mristin/opinionated-commit-message/' + + 'blob/master/src/mostFrequentEnglishVerbs.ts.' + ]; + if (!inputs.hasAdditionalVerbsInput) { + parts.push('You can whitelist additional verbs using ' + + '"additional-verbs" input to your GitHub action ' + + '(currently no additional verbs were thus specified).'); + } + else { + parts.push('You can whitelist additional verbs using ' + + '"additional-verbs" input to your GitHub action ' + + `(currently one or more additional verbs were thus ` + + 'specified).'); + } + if (inputs.pathToAdditionalVerbs.length === 0) { + parts.push('Moreover, you can also whitelist additional verbs in a file ' + + 'given as "path-to-additional-verbs" input to your GitHub action ' + + '(currently no whitelist file was specified).'); + } + else { + parts.push('Moreover, you can also whitelist additional verbs in a file ' + + 'given as "path-to-additional-verbs" input to your GitHub action ' + + `(currently the file is: ${inputs.pathToAdditionalVerbs}).`); + } + parts.push('Please check the whitelist and either change the first word ' + + 'of the subject or whitelist the verb.'); + return parts.join(' '); +} +function checkSubject(subject, inputs) { // Pre-condition - for (const verb of additionalVerbs) { + for (const verb of inputs.additionalVerbs) { if (verb.length === 0) { throw new Error(`Unexpected empty additional verb`); } @@ -591,46 +652,46 @@ function checkSubject(subject, additionalVerbs) { } const errors = []; // Tolerate the hash code referring, e.g., to a pull request. - // These hash codes are usually added automatically by Github and + // These hash codes are usually added automatically by GitHub and // similar services. const subjectWoCode = subject.replace(suffixHashCodeRe, ''); if (subjectWoCode.length > 50) { errors.push(`The subject exceeds the limit of 50 characters ` + - `(got: ${subject.length}, JSONified: ${JSON.stringify(subjectWoCode)})`); + `(got: ${subject.length}, JSON: ${JSON.stringify(subjectWoCode)}).` + + 'Please shorten the subject to make it more succinct.'); } - const match = capitalizedWordRe.exec(subjectWoCode); - if (!match) { - errors.push('The subject must start with a capitalized verb (e.g., "Change").'); + const firstWord = extractFirstWord(subjectWoCode); + if (!firstWord) { + errors.push('Expected the subject to start with a verb in imperative mood ' + + 'consisting of letters and possibly dashes in-between, ' + + `but the subject was: ${JSON.stringify(subjectWoCode)}. ` + + 'Please re-write the subject so that it starts with ' + + 'a verb in imperative mood.'); } else { - if (match.length < 2) { - throw Error('Expected at least one group to match the first capitalized word, ' + - 'but got none.'); - } - const word = match[1]; - if (!mostFrequentEnglishVerbs.SET.has(word.toLowerCase()) && - !additionalVerbs.has(word.toLowerCase())) { - if (additionalVerbs.size === 0) { - errors.push('The subject must start in imperative mood with one of the ' + - `most frequent English verbs, but got: ${JSON.stringify(word)}. ` + - 'Please see ' + - 'https://github.com/mristin/opinionated-commit-message/blob/master/' + - 'src/mostFrequentEnglishVerbs.ts ' + - 'for a complete list.'); - } - else { - errors.push('The subject must start in imperative mood with one of the ' + - `most frequent English verbs, but got: ${JSON.stringify(word)}. ` + - 'Please see ' + - 'https://github.com/mristin/opinionated-commit-message/blob/master/' + - 'src/mostFrequentEnglishVerbs.ts ' + - 'for a complete list and ' + - 'also revisit your list of additional verbs.'); - } + const capitalized = capitalize(firstWord); + if (firstWord !== capitalized) { + errors.push('The subject must start with a capitalized word, ' + + `but the current first word is: ${JSON.stringify(firstWord)}. ` + + `Please capitalize to: ${JSON.stringify(capitalized)}.`); + } + if (!mostFrequentEnglishVerbs.SET.has(firstWord.toLowerCase()) && + !inputs.additionalVerbs.has(firstWord.toLowerCase())) { + /* + (mristin, 2020-09-09): It might be worthwhile to refactor the rendering + of the error messages to a separate module and use classes to represent + the errors. The complexity is still manageable, so it is not yet the + moment to do so since the refactoring would be quite time-consuming. + + Originally, I did not foresee that error messages need such a large + information content. + */ + errors.push(errorMessageOnNonVerb(firstWord, inputs)); } } if (subjectWoCode.endsWith('.')) { - errors.push("The subject must not end with a dot ('.')."); + errors.push("The subject must not end with a dot ('.'). " + + 'Please remove the trailing dot(s).'); } return errors; } @@ -654,22 +715,24 @@ function checkBody(subject, bodyLines) { errors.push(`The line ${i + 3} of the message (line ${i + 1} of the body) ` + 'exceeds the limit of 72 characters. ' + `The line contains ${line.length} characters: ` + - `${JSON.stringify(line)}.`); + `${JSON.stringify(line)}. ` + + 'Please reformat the body so that all the lines fit ' + + '72 characters.'); } } - const bodyFirstWordMatch = capitalizedWordRe.exec(bodyLines[0]); - if (bodyFirstWordMatch) { - const bodyFirstWord = bodyFirstWordMatch[1]; - const subjectFirstWordMatch = capitalizedWordRe.exec(subject); - if (subjectFirstWordMatch !== undefined && - subjectFirstWordMatch !== null && - subjectFirstWordMatch.length > 0) { - const subjectFirstWord = subjectFirstWordMatch[1]; + const bodyFirstWord = extractFirstWord(bodyLines[0]); + if (bodyFirstWord) { + const subjectFirstWord = extractFirstWord(subject); + if (subjectFirstWord) { if (subjectFirstWord.toLowerCase() === bodyFirstWord.toLowerCase()) { errors.push('The first word of the subject ' + `(${JSON.stringify(subjectFirstWord)}) ` + - 'must not match ' + - 'the first word of the body.'); + 'must not match the first word of the body. ' + + 'Please make the body more informative by adding more ' + + 'information instead of repeating the subject. ' + + 'For example, start by explaining the problem that this change ' + + 'is intended to solve or what was previously missing ' + + '(e.g., "Previously, ....").'); } } } @@ -677,7 +740,7 @@ function checkBody(subject, bodyLines) { } const mergeMessageRe = new RegExp("^Merge branch '[^\\000-\\037\\177 ~^:?*[]+' " + 'into [^\\000-\\037\\177 ~^:?*[]+$'); -function check(message, additionalVerbs, allowOneLiners) { +function check(message, inputs) { const errors = []; if (mergeMessageRe.test(message)) { return errors; @@ -687,8 +750,8 @@ function check(message, additionalVerbs, allowOneLiners) { errors.push(`The message is empty.`); return errors; } - else if (lines.length === 1 && allowOneLiners) { - errors.push(...checkSubject(lines[0], additionalVerbs)); + else if (lines.length === 1 && inputs.allowOneLiners) { + errors.push(...checkSubject(lines[0], inputs)); } else { const maybeSubjectBody = splitSubjectBody(lines); @@ -700,7 +763,7 @@ function check(message, additionalVerbs, allowOneLiners) { throw Error('Unexpected undefined subjectBody'); } const subjectBody = maybeSubjectBody.subjectBody; - errors.push(...checkSubject(subjectBody.subject, additionalVerbs)); + errors.push(...checkSubject(subjectBody.subject, inputs)); errors.push(...checkBody(subjectBody.subject, subjectBody.bodyLines)); } } @@ -4507,64 +4570,36 @@ var __importStar = (this && this.__importStar) || function (mod) { __setModuleDefault(result, mod); return result; }; -var __importDefault = (this && this.__importDefault) || function (mod) { - return (mod && mod.__esModule) ? mod : { "default": mod }; -}; Object.defineProperty(exports, "__esModule", { value: true }); exports.run = void 0; -const fs_1 = __importDefault(__webpack_require__(747)); const core = __importStar(__webpack_require__(470)); const commitMessages = __importStar(__webpack_require__(281)); const inspection = __importStar(__webpack_require__(117)); const represent = __importStar(__webpack_require__(110)); const input = __importStar(__webpack_require__(553)); function runWithExceptions() { + var _a, _b, _c; const messages = commitMessages.retrieve(); - const additionalVerbs = new Set(); - // Parse additional-verbs input - const additionalVerbsInput = core.getInput('additional-verbs', { - required: false - }); - if (additionalVerbsInput) { - for (const verb of input.parseVerbs(additionalVerbsInput)) { - additionalVerbs.add(verb); - } - } - // Parse additional-verbs-from-path input - const pathToAdditionalVerbs = core.getInput('path-to-additional-verbs', { - required: false - }); - if (pathToAdditionalVerbs) { - if (!fs_1.default.existsSync(pathToAdditionalVerbs)) { - const error = 'The file referenced by path-to-additional-verbs could ' + - `not be found: ${pathToAdditionalVerbs}`; - core.error(error); - core.setFailed(error); - return; - } - const text = fs_1.default.readFileSync(pathToAdditionalVerbs).toString('utf-8'); - for (const verb of input.parseVerbs(text)) { - additionalVerbs.add(verb); - } - } - // Parse allow-one-liners input - const allowOneLinersText = core.getInput('allow-one-liners', { - required: false - }); - const allowOneLiners = !allowOneLinersText - ? false - : input.parseAllowOneLiners(allowOneLinersText); - if (allowOneLiners === null) { - const error = 'Unexpected value for allow-one-liners. ' + - `Expected either 'true' or 'false', got: ${allowOneLinersText}`; - core.error(error); - core.setFailed(error); + //// + // Parse inputs + //// + const additionalVerbsInput = (_a = core.getInput('additional-verbs', { required: false })) !== null && _a !== void 0 ? _a : ''; + const pathToAdditionalVerbsInput = (_b = core.getInput('path-to-additional-verbs', { required: false })) !== null && _b !== void 0 ? _b : ''; + const allowOneLinersInput = (_c = core.getInput('allow-one-liners', { required: false })) !== null && _c !== void 0 ? _c : ''; + const maybeInputs = input.parseInputs(additionalVerbsInput, pathToAdditionalVerbsInput, allowOneLinersInput); + if (maybeInputs.error !== null) { + core.error(maybeInputs.error); + core.setFailed(maybeInputs.error); return; } + const inputs = maybeInputs.mustInputs(); + //// + // Inspect + //// // Parts of the error message to be concatenated with '\n' const parts = []; for (const [messageIndex, message] of messages.entries()) { - const errors = inspection.check(message, additionalVerbs, allowOneLiners); + const errors = inspection.check(message, inputs); if (errors.length > 0) { const repr = represent.formatErrors(message, messageIndex, errors); parts.push(repr); @@ -8766,7 +8801,10 @@ exports.SET = new Set([ 'configure', 're-instate', 'reinstate', - 'pin' + 'pin', + 'hint', + 'integrate', + 'instruct' ]); @@ -9326,12 +9364,73 @@ function getNextPage (octokit, link, headers) { /***/ }), /***/ 553: -/***/ (function(__unusedmodule, exports) { +/***/ (function(__unusedmodule, exports, __webpack_require__) { "use strict"; +var __importDefault = (this && this.__importDefault) || function (mod) { + return (mod && mod.__esModule) ? mod : { "default": mod }; +}; Object.defineProperty(exports, "__esModule", { value: true }); -exports.parseAllowOneLiners = exports.parseVerbs = void 0; +exports.parseAllowOneLiners = exports.parseVerbs = exports.parseInputs = exports.MaybeInputs = exports.Inputs = void 0; +const fs_1 = __importDefault(__webpack_require__(747)); +class Inputs { + constructor(hasAdditionalVerbsInput, pathToAdditionalVerbs, allowOneLiners, additionalVerbs) { + this.hasAdditionalVerbsInput = hasAdditionalVerbsInput; + this.pathToAdditionalVerbs = pathToAdditionalVerbs; + this.allowOneLiners = allowOneLiners; + this.additionalVerbs = additionalVerbs; + } +} +exports.Inputs = Inputs; +class MaybeInputs { + constructor(inputs, error) { + if (inputs === null && error === null) { + throw Error("Unexpected both 'inputs' and 'error' arguments to be null."); + } + if (inputs !== null && error !== null) { + throw Error("Unexpected both 'inputs' and 'error' arguments to be given."); + } + this.inputs = inputs; + this.error = error; + } + mustInputs() { + if (this.inputs === null) { + throw Error("The field 'inputs' is expected to be set, but it is null. " + + `The field 'error' is: ${this.error}`); + } + return this.inputs; + } +} +exports.MaybeInputs = MaybeInputs; +function parseInputs(additionalVerbsInput, pathToAdditionalVerbsInput, allowOneLinersInput) { + const additionalVerbs = new Set(); + const hasAdditionalVerbsInput = additionalVerbsInput.length > 0; + if (additionalVerbsInput) { + for (const verb of parseVerbs(additionalVerbsInput)) { + additionalVerbs.add(verb); + } + } + if (pathToAdditionalVerbsInput) { + if (!fs_1.default.existsSync(pathToAdditionalVerbsInput)) { + return new MaybeInputs(null, 'The file referenced by path-to-additional-verbs could ' + + `not be found: ${pathToAdditionalVerbsInput}`); + } + const text = fs_1.default.readFileSync(pathToAdditionalVerbsInput).toString('utf-8'); + for (const verb of parseVerbs(text)) { + additionalVerbs.add(verb); + } + } + const allowOneLiners = !allowOneLinersInput + ? false + : parseAllowOneLiners(allowOneLinersInput); + if (allowOneLiners === null) { + return new MaybeInputs(null, 'Unexpected value for allow-one-liners. ' + + `Expected either 'true' or 'false', got: ${allowOneLinersInput}`); + } + return new MaybeInputs(new Inputs(hasAdditionalVerbsInput, pathToAdditionalVerbsInput, allowOneLiners, additionalVerbs), null); +} +exports.parseInputs = parseInputs; function parseVerbs(text) { const lines = text.split('\n'); const verbs = []; diff --git a/local/powershell/OpinionatedCommitMessage.Tests.ps1 b/local/powershell/OpinionatedCommitMessage.Tests.ps1 index 1dd253b..2545e53 100755 --- a/local/powershell/OpinionatedCommitMessage.Tests.ps1 +++ b/local/powershell/OpinionatedCommitMessage.Tests.ps1 @@ -30,13 +30,62 @@ function TestFailTooFewLines Write-Host "TestFailTooFewLines: OK" } +function TestFailSubjectWithNonWord +{ + $message = "ABC15 does something`n`nThis patch does something." + $got = (powershell -File OpinionatedCommitMessage.ps1 -message $message -dontThrow)|Out-String + + $nl = [Environment]::NewLine + $expected = ( + "* Expected the subject to start with a verb in imperative mood consisting " + + "of letters and possibly dashes in-between, but the subject was: `"ABC15 does something`". " + + "Please re-write the subject so that it starts with a verb in imperative mood.${nl}" + ) + + if ($got -ne $expected) + { + Write-Host "TestFailSubjectWithNonWord: FAILED" + WriteExpectedGot -expected $expected -got $got + return $false + } + + Write-Host "TestFailSubjectWithNonWord: OK" + return $true +} + +function TestFailSubjectWithNonCapitalized +{ + $message = "do something`n`nThis patch does something." + $got = (powershell -File OpinionatedCommitMessage.ps1 -message $message -dontThrow)|Out-String + + $nl = [Environment]::NewLine + $expected = ( + "* The subject must start with a capitalized word, but the current first word is: `"do`". " + + "Please capitalize to: `"Do`".${nl}" + ) + + if ($got -ne $expected) + { + Write-Host "TestFailSubjectWithNonCapitalized: FAILED" + WriteExpectedGot -expected $expected -got $got + return $false + } + + Write-Host "TestFailSubjectWithNonCapitalized: OK" + return $true +} + function TestFailSubjectWithNonVerb { - $message = "ABC does something`n`nThis patch does something." + $message = "Abc does something`n`nThis patch does something." $got = (powershell -File OpinionatedCommitMessage.ps1 -message $message -dontThrow)|Out-String $nl = [Environment]::NewLine - $expected = "* The subject must start with a capitalized verb (e.g., `"Change`").${nl}" + $expected = ( + "* The subject must start with a verb in imperative mood (according to a whitelist), " + + "but got: `"Abc`"; if this is a false positive, consider adding the verb to -additionalVerbs or " + + "to the file referenced by -pathToAdditionalVerbs.${nl}" + ) if ($got -ne $expected) { @@ -55,7 +104,7 @@ function TestFailSubjectWithTrailingDot $got = (powershell -File OpinionatedCommitMessage.ps1 -message $message -dontThrow)|Out-String $nl = [Environment]::NewLine - $expected = "* The subject must not end with a dot ('.').${nl}" + $expected = "* The subject must not end with a dot ('.'). Please remove the trailing dot(s).${nl}" if ($got -ne $expected) { @@ -74,9 +123,17 @@ function TestFailSubjectTooLong $got = (powershell -File OpinionatedCommitMessage.ps1 -message $message -dontThrow)|Out-String $nl = [Environment]::NewLine - $expected = "* The subject exceeds the limit of 50 characters " + ` - "(got: 51): `"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA`"${nl}" + ` - "* The subject must start with a capitalized verb (e.g., `"Change`").${nl}" + $expected = ( + "* The subject exceeds the limit of 50 characters " + + "(got: 51): `"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA`". " + + "Please shorten the subject to make it more succinct.${nl}" + + "* The subject must start with a capitalized word, but " + + "the current first word is: `"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA`". " + + "Please capitalize to: `"Aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa`".${nl}" + + "* The subject must start with a verb in imperative mood (according to a whitelist), " + + "but got: `"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA`"; if this is a false positive, " + + "consider adding the verb to -additionalVerbs or to the file referenced by -pathToAdditionalVerbs.${nl}" + ) if ($got -ne $expected) { @@ -96,8 +153,11 @@ function TestFailBodyTooLong $got = (powershell -File OpinionatedCommitMessage.ps1 -message $message -dontThrow)|Out-String $nl = [Environment]::NewLine - $expected = "* The line 3 of the message (line 1 of the body) exceeds the limit of 72 characters. " + ` - "The line contains 73 characters: `"${bodyLine}`".${nl}" + $expected = ( + "* The line 3 of the message (line 1 of the body) exceeds the limit of 72 characters. " + + "The line contains 73 characters: `"${bodyLine}`". " + + "Please reformat the body so that all the lines fit 72 characters.${nl}" + ) if ($got -ne $expected) { @@ -116,7 +176,12 @@ function TestFailIdenticalFirstWordInBodyAndSubject $got = (powershell -File OpinionatedCommitMessage.ps1 -message $message -dontThrow)|Out-String $nl = [Environment]::NewLine - $expected = "* The first word of the subject (`"Do`") must not match the first word of the body.${nl}" + $expected = ( + "* The first word of the subject (`"Do`") must not match the first word of the body. " + + "Please make the body more informative by adding more information instead of repeating " + + "the subject. For example, start by explaining the problem that this change is " + + 'intendended to solve or what was previously missing (e.g., "Previously, ....").' + $nl + ) if ($got -ne $expected) { @@ -280,7 +345,7 @@ function TestFailWithAllowOneLiners )|Out-String $nl = [Environment]::NewLine - $expected = "* The subject must not end with a dot ('.').${nl}" + $expected = "* The subject must not end with a dot ('.'). Please remove the trailing dot(s).${nl}" if ($got -ne $expected) { @@ -345,7 +410,10 @@ function Main try { - $success = TestFailTooFewLines + $success = $true + $success = TestFailTooFewLines -and $success + $success = TestFailSubjectWithNonWord -and $success + $success = TestFailSubjectWithNonCapitalized -and $success $success = TestFailSubjectWithNonVerb -and $success $success = TestFailSubjectWithTrailingDot -and $success $success = TestFailSubjectTooLong -and $success diff --git a/local/powershell/OpinionatedCommitMessage.ps1 b/local/powershell/OpinionatedCommitMessage.ps1 index 02a22b9..9052352 100644 --- a/local/powershell/OpinionatedCommitMessage.ps1 +++ b/local/powershell/OpinionatedCommitMessage.ps1 @@ -410,6 +410,7 @@ $frequentVerbs = @( 'inline', 'insist', 'install', +'instruct', 'integrate', 'intend', 'intercept', @@ -817,9 +818,47 @@ function ParseAdditionalVerbs($text) return $verbs } -$capitalizedWordRe = [Regex]::new('^([A-Z][a-z]*)[^a-zA-Z]') +$allLettersRe = [Regex]::new('^[a-zA-Z][a-zA-Z-]+$') +$firstWordBeforeSpaceRe = [Regex]::new('^([a-zA-Z][a-zA-Z-]+)\s'); $suffixHashCodeRe = [Regex]::new('\s?\(\s*#[a-zA-Z_0-9]+\s*\)$') +function ExtractFirstWord([string]$Text) +{ + if ($Text.Length -eq 0) + { + return $null + } + + if ($allLettersRe.IsMatch($Text)) + { + return $Text + } + + $matches = $firstWordBeforeSpaceRe.Match($Text) + if (!$matches.Success) + { + return $null + } + + $firstWord = $matches[0].Groups[1].Value + return $firstWord +} + +function Capitalize([string]$word) +{ + if ($word.Length -eq 0) + { + return "" + } + + if ($word.Length -eq 1) + { + return $word.ToUpperInvariant() + } + + return $word.Substring(0, 1).ToUpperInvariant() + $word.Substring(1).ToLowerInvariant() +} + function CheckSubject([string]$subject, [hashtable]$verbs) { # Precondition @@ -842,31 +881,47 @@ function CheckSubject([string]$subject, [hashtable]$verbs) if ($subjectWoCode.Length -gt 50) { $errors += "The subject exceeds the limit of 50 characters " + ` - "(got: $( $subjectWoCode.Length )): $( $subjectWoCode|ConvertTo-Json )" + "(got: $( $subjectWoCode.Length )): $( $subjectWoCode|ConvertTo-Json ). " + + "Please shorten the subject to make it more succinct." } - $matches = $capitalizedWordRe.Matches($subjectWoCode) - if ($matches.Count -ne 1) + $firstWord = ExtractFirstWord -Text $subjectWoCode + if ($null -eq $firstWord) { - $errors += 'The subject must start with a capitalized verb (e.g., "Change").' - } - else - { - $match = $matches[0] - $word = $match.Groups[1].Value - $wordLower = $word.ToLower() + $errors += ( + "Expected the subject to start with a verb in imperative mood " + + "consisting of letters and possibly dashes in-between, " + + "but the subject was: $($subjectWoCode|ConvertTo-Json). " + + "Please re-write the subject so that it starts with " + + "a verb in imperative mood." + ) + } else { + $capitalized = Capitalize -Word $firstWord + if ($capitalized -cne $firstWord) + { + $errors += ( + "The subject must start with a capitalized word, " + + "but the current first word is: $( $firstWord|ConvertTo-Json ). " + + "Please capitalize to: $( $capitalized|ConvertTo-Json )." + ) + } + + $firstWordLower = $firstWord.ToLower() - if (!$verbs.Contains($wordLower) -or ($false -eq $verbs[$wordLower])) + if (!$verbs.Contains($firstWordLower) -or ($false -eq $verbs[$firstWordLower])) { $errors += "The subject must start with a verb in imperative mood (according to a whitelist), " + ` - "but got: $($word|ConvertTo-Json); if this is a false positive, consider adding the verb " + ` + "but got: $($firstWord|ConvertTo-Json); if this is a false positive, consider adding the verb " + ` "to -additionalVerbs or to the file referenced by -pathToAdditionalVerbs." } } if ( $subjectWoCode.EndsWith(".")) { - $errors += "The subject must not end with a dot ('.')." + $errors += ( + "The subject must not end with a dot ('.'). " + + "Please remove the trailing dot(s)." + ) } return $errors @@ -902,26 +957,30 @@ function CheckBody([string]$subject, [string[]] $bodyLines) if($line.Length -gt 72) { - $errors += "The line $($i + 3) of the message (line $($i + 1) of the body) " + ` - "exceeds the limit of 72 characters. The line contains $($line.Length) characters: " + ` - "$($line|ConvertTo-Json)." + $errors += ( + "The line $($i + 3) of the message (line $($i + 1) of the body) " + + "exceeds the limit of 72 characters. The line contains $($line.Length) characters: " + + "$($line|ConvertTo-Json). " + + "Please reformat the body so that all the lines fit 72 characters." + ) } } - $bodyFirstWordMatches = $capitalizedWordRe.Matches($bodyLines[0]) - if($bodyFirstWordMatches.Count -eq 1) + $bodyFirstWord = ExtractFirstWord -Text $bodyLines[0] + if($null -ne $bodyFirstWord) { - $bodyFirstWord = $bodyFirstWordMatches[0].Groups[1].Value - - $subjectFirstWordMatches = $capitalizedWordRe.Matches($subject) - if($subjectFirstWordMatches.Count -eq 1) + $subjectFirstWord = ExtractFirstWord -Text $subject + if($null -ne $subjectFirstWord) { - $subjectFirstWord = $subjectFirstWordMatches[0].Groups[1].Value - if($subjectFirstWord.ToLower() -eq $bodyFirstWord.ToLower()) { - $errors += "The first word of the subject ($($subjectFirstWord|ConvertTo-Json)) must not match " + ` - "the first word of the body." + $errors += ( + "The first word of the subject ($($subjectFirstWord|ConvertTo-Json)) must not match " + + "the first word of the body. " + + "Please make the body more informative by adding more information instead of repeating " + + "the subject. For example, start by explaining the problem that this change is " + + 'intendended to solve or what was previously missing (e.g., "Previously, ....").' + ) } } } diff --git a/local/powershell/OpinionatedCommitMessage.ps1.template b/local/powershell/OpinionatedCommitMessage.ps1.template index 2af3185..04ca134 100755 --- a/local/powershell/OpinionatedCommitMessage.ps1.template +++ b/local/powershell/OpinionatedCommitMessage.ps1.template @@ -77,9 +77,47 @@ function ParseAdditionalVerbs($text) return $verbs } -$capitalizedWordRe = [Regex]::new('^([A-Z][a-z]*)[^a-zA-Z]') +$allLettersRe = [Regex]::new('^[a-zA-Z][a-zA-Z-]+$') +$firstWordBeforeSpaceRe = [Regex]::new('^([a-zA-Z][a-zA-Z-]+)\s'); $suffixHashCodeRe = [Regex]::new('\s?\(\s*#[a-zA-Z_0-9]+\s*\)$') +function ExtractFirstWord([string]$Text) +{ + if ($Text.Length -eq 0) + { + return $null + } + + if ($allLettersRe.IsMatch($Text)) + { + return $Text + } + + $matches = $firstWordBeforeSpaceRe.Match($Text) + if (!$matches.Success) + { + return $null + } + + $firstWord = $matches[0].Groups[1].Value + return $firstWord +} + +function Capitalize([string]$word) +{ + if ($word.Length -eq 0) + { + return "" + } + + if ($word.Length -eq 1) + { + return $word.ToUpperInvariant() + } + + return $word.Substring(0, 1).ToUpperInvariant() + $word.Substring(1).ToLowerInvariant() +} + function CheckSubject([string]$subject, [hashtable]$verbs) { # Precondition @@ -102,31 +140,47 @@ function CheckSubject([string]$subject, [hashtable]$verbs) if ($subjectWoCode.Length -gt 50) { $errors += "The subject exceeds the limit of 50 characters " + ` - "(got: $( $subjectWoCode.Length )): $( $subjectWoCode|ConvertTo-Json )" + "(got: $( $subjectWoCode.Length )): $( $subjectWoCode|ConvertTo-Json ). " + + "Please shorten the subject to make it more succinct." } - $matches = $capitalizedWordRe.Matches($subjectWoCode) - if ($matches.Count -ne 1) + $firstWord = ExtractFirstWord -Text $subjectWoCode + if ($null -eq $firstWord) { - $errors += 'The subject must start with a capitalized verb (e.g., "Change").' - } - else - { - $match = $matches[0] - $word = $match.Groups[1].Value - $wordLower = $word.ToLower() + $errors += ( + "Expected the subject to start with a verb in imperative mood " + + "consisting of letters and possibly dashes in-between, " + + "but the subject was: $($subjectWoCode|ConvertTo-Json). " + + "Please re-write the subject so that it starts with " + + "a verb in imperative mood." + ) + } else { + $capitalized = Capitalize -Word $firstWord + if ($capitalized -cne $firstWord) + { + $errors += ( + "The subject must start with a capitalized word, " + + "but the current first word is: $( $firstWord|ConvertTo-Json ). " + + "Please capitalize to: $( $capitalized|ConvertTo-Json )." + ) + } + + $firstWordLower = $firstWord.ToLower() - if (!$verbs.Contains($wordLower) -or ($false -eq $verbs[$wordLower])) + if (!$verbs.Contains($firstWordLower) -or ($false -eq $verbs[$firstWordLower])) { $errors += "The subject must start with a verb in imperative mood (according to a whitelist), " + ` - "but got: $($word|ConvertTo-Json); if this is a false positive, consider adding the verb " + ` + "but got: $($firstWord|ConvertTo-Json); if this is a false positive, consider adding the verb " + ` "to -additionalVerbs or to the file referenced by -pathToAdditionalVerbs." } } if ( $subjectWoCode.EndsWith(".")) { - $errors += "The subject must not end with a dot ('.')." + $errors += ( + "The subject must not end with a dot ('.'). " + + "Please remove the trailing dot(s)." + ) } return $errors @@ -162,26 +216,30 @@ function CheckBody([string]$subject, [string[]] $bodyLines) if($line.Length -gt 72) { - $errors += "The line $($i + 3) of the message (line $($i + 1) of the body) " + ` - "exceeds the limit of 72 characters. The line contains $($line.Length) characters: " + ` - "$($line|ConvertTo-Json)." + $errors += ( + "The line $($i + 3) of the message (line $($i + 1) of the body) " + + "exceeds the limit of 72 characters. The line contains $($line.Length) characters: " + + "$($line|ConvertTo-Json). " + + "Please reformat the body so that all the lines fit 72 characters." + ) } } - $bodyFirstWordMatches = $capitalizedWordRe.Matches($bodyLines[0]) - if($bodyFirstWordMatches.Count -eq 1) + $bodyFirstWord = ExtractFirstWord -Text $bodyLines[0] + if($null -ne $bodyFirstWord) { - $bodyFirstWord = $bodyFirstWordMatches[0].Groups[1].Value - - $subjectFirstWordMatches = $capitalizedWordRe.Matches($subject) - if($subjectFirstWordMatches.Count -eq 1) + $subjectFirstWord = ExtractFirstWord -Text $subject + if($null -ne $subjectFirstWord) { - $subjectFirstWord = $subjectFirstWordMatches[0].Groups[1].Value - if($subjectFirstWord.ToLower() -eq $bodyFirstWord.ToLower()) { - $errors += "The first word of the subject ($($subjectFirstWord|ConvertTo-Json)) must not match " + ` - "the first word of the body." + $errors += ( + "The first word of the subject ($($subjectFirstWord|ConvertTo-Json)) must not match " + + "the first word of the body. " + + "Please make the body more informative by adding more information instead of repeating " + + "the subject. For example, start by explaining the problem that this change is " + + 'intendended to solve or what was previously missing (e.g., "Previously, ....").' + ) } } } diff --git a/src/__tests__/input.test.ts b/src/__tests__/input.test.ts index a06a2dd..807b6f4 100644 --- a/src/__tests__/input.test.ts +++ b/src/__tests__/input.test.ts @@ -1,4 +1,5 @@ import * as input from '../input'; +import fs from 'fs'; it('parses commas, semi-colons and newlines.', () => { const text = 'one, two; three\nfour\n'; @@ -15,3 +16,33 @@ it('trims before and after.', () => { expect(verbs).toEqual(['one', 'two', 'three', 'four']); }); + +it('parses the inputs.', () => { + const pathToVerbs = '/some/path/to/additional/verbs'; + + (fs as any).existsSync = (path: string) => path === pathToVerbs; + + (fs as any).readFileSync = (path: string) => { + if (path === pathToVerbs) { + return 'rewrap\ntable'; + } + + throw new Error(`Unexpected readFileSync in the unit test from: ${path}`); + }; + + const maybeInputs = input.parseInputs( + 'integrate\nanalyze', + pathToVerbs, + 'true' + ); + + expect(maybeInputs.error).toBeNull(); + + const inputs = maybeInputs.mustInputs(); + expect(inputs.hasAdditionalVerbsInput).toBeTruthy(); + expect(inputs.pathToAdditionalVerbs).toEqual(pathToVerbs); + expect(inputs.additionalVerbs).toEqual( + new Set(['rewrap', 'table', 'integrate', 'analyze']) + ); + expect(inputs.allowOneLiners).toBeTruthy(); +}); diff --git a/src/__tests__/inspection.test.ts b/src/__tests__/inspection.test.ts index 53b45f2..ec3013d 100644 --- a/src/__tests__/inspection.test.ts +++ b/src/__tests__/inspection.test.ts @@ -1,5 +1,8 @@ +import * as input from '../input'; import * as inspection from '../inspection'; +const defaultInputs: input.Inputs = input.parseInputs('', '', '').mustInputs(); + it('reports no errors on correct multi-line message.', () => { const message = 'Change SomeClass to OtherClass\n' + @@ -7,7 +10,7 @@ it('reports no errors on correct multi-line message.', () => { 'This replaces the SomeClass with OtherClass in all of the module \n' + 'since Some class was deprecated.'; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([]); }); @@ -18,14 +21,16 @@ it('reports no errors on OK multi-line message with allowed one-liners.', () => 'This replaces the SomeClass with OtherClass in all of the module \n' + 'since Some class was deprecated.'; - const errors = inspection.check(message, new Set(), true); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([]); }); it('reports no errors on OK single-line message with allowed one-liners.', () => { + const inputs = input.parseInputs('', '', 'true').mustInputs(); + const message = 'Change SomeClass to OtherClass'; - const errors = inspection.check(message, new Set(), true); + const errors = inspection.check(message, inputs); expect(errors).toEqual([]); }); @@ -36,13 +41,13 @@ it('tolerates hash code in the subject.', () => { 'The license files naming was inconsistent (`LICENSE.TXT` and \n' + '`LICENSE.txt`). This makes them all uniform (`LICENSE.txt`).'; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([]); }); it('reports too few lines with disallowed one-liners.', () => { const message = 'Change SomeClass to OtherClass'; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([ 'Expected at least three lines (subject, empty, body), but got: 1' ]); @@ -50,13 +55,15 @@ it('reports too few lines with disallowed one-liners.', () => { it('reports missing body with disallowed one-liners.', () => { const message = 'Change SomeClass to OtherClass\n\n'; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual(['Unexpected empty body']); }); it('reports missing body with allowed one-liners.', () => { + const inputs = input.parseInputs('', '', 'true').mustInputs(); + const message = 'Change SomeClass to OtherClass\n'; - const errors = inspection.check(message, new Set(), true); + const errors = inspection.check(message, inputs); expect(errors).toEqual([ 'Expected at least three lines (subject, empty, body) ' + 'in a multi-line message, but got: 2' @@ -70,23 +77,38 @@ it('reports on missing empty line between subject and body.', () => { 'This replaces the SomeClass with OtherClass in all of the module \n' + 'since Some class was deprecated.'; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([ 'Expected an empty line between the subject and the body, ' + 'but got a second line of length: 3' ]); }); +it('reports the subject starting with a non-word.', () => { + const message = 'ABC12\n\nThis does something.'; + + const errors = inspection.check(message, defaultInputs); + expect(errors).toEqual([ + 'Expected the subject to start with a verb in imperative mood ' + + 'consisting of letters and possibly dashes in-between, ' + + 'but the subject was: "ABC12". ' + + 'Please re-write the subject so that it starts ' + + 'with a verb in imperative mood.' + ]); +}); + it('reports the subject starting with a non-capitalized word.', () => { const message = - 'SomeClass to OtherClass\n' + + 'change SomeClass to OtherClass\n' + '\n' + 'This replaces the SomeClass with OtherClass in all of the module \n' + 'since Some class was deprecated.'; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([ - 'The subject must start with a capitalized verb (e.g., "Change").' + 'The subject must start with a capitalized word, ' + + 'but the current first word is: "change". ' + + 'Please capitalize to: "Change".' ]); }); @@ -100,39 +122,102 @@ it( 'This replaces the SomeClass with OtherClass in all of the module \n' + 'since Some class was deprecated.'; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors.length).toBe(1); - expect(errors[0].startsWith('The subject must start in imperative mood')); + expect(errors).toEqual([ + 'The subject must start with a verb in imperative mood, ' + + 'but it started with: "Replaced". ' + + 'Whether the word is in imperative mood is determined by ' + + 'whitelisting. The general whitelist is available at ' + + 'https://github.com/mristin/opinionated-commit-message/' + + 'blob/master/src/mostFrequentEnglishVerbs.ts. ' + + 'You can whitelist additional verbs using "additional-verbs" input ' + + 'to your GitHub action (currently no additional verbs were thus ' + + 'specified). Moreover, you can also whitelist additional verbs ' + + 'in a file given as "path-to-additional-verbs" input to ' + + 'your GitHub action (currently no whitelist file was specified). ' + + 'Please check the whitelist and either change the first word ' + + 'of the subject or whitelist the verb.' + ]); } ); it( 'reports the subject starting with a non-verb ' + - 'with additional verbs given.', + 'with additional verbs given as direct input.', () => { + const inputs = input.parseInputs('table', '', 'false').mustInputs(); + const message = 'Replaced SomeClass to OtherClass\n' + '\n' + 'This replaces the SomeClass with OtherClass in all of the module \n' + 'since Some class was deprecated.'; - const errors = inspection.check( - message, - new Set(['table']), - false + const errors = inspection.check(message, inputs); + + expect(errors.length).toBe(1); + expect(errors).toEqual([ + 'The subject must start with a verb in imperative mood, ' + + 'but it started with: "Replaced". ' + + 'Whether the word is in imperative mood is ' + + 'determined by whitelisting. The general whitelist is available at ' + + 'https://github.com/mristin/opinionated-commit-message/' + + 'blob/master/src/mostFrequentEnglishVerbs.ts. You can whitelist ' + + 'additional verbs using "additional-verbs" input to your GitHub ' + + 'action (currently one or more additional verbs were thus specified). ' + + 'Moreover, you can also whitelist additional verbs in a file given ' + + 'as "path-to-additional-verbs" input to your GitHub action ' + + '(currently no whitelist file was specified). Please check the ' + + 'whitelist and either change the first word of the subject or ' + + 'whitelist the verb.' + ]); + } +); + +it( + 'reports the subject starting with a non-verb ' + + 'with additional verbs given in a path.', + () => { + const inputs = new input.Inputs( + false, + '/some/path', + false, + new Set('table') ); + + const message = + 'Replaced SomeClass to OtherClass\n' + + '\n' + + 'This replaces the SomeClass with OtherClass in all of the module \n' + + 'since Some class was deprecated.'; + + const errors = inspection.check(message, inputs); + expect(errors.length).toBe(1); - expect(errors[0].startsWith('The subject must start in imperative mood')); + expect(errors).toEqual([ + 'The subject must start with a verb in imperative mood, ' + + 'but it started with: "Replaced". ' + + 'Whether the word is in imperative mood is ' + + 'determined by whitelisting. The general whitelist is available at ' + + 'https://github.com/mristin/opinionated-commit-message/' + + 'blob/master/src/mostFrequentEnglishVerbs.ts. You can whitelist ' + + 'additional verbs using "additional-verbs" input to your GitHub ' + + 'action (currently no additional verbs were thus specified). ' + + 'Moreover, you can also whitelist additional verbs in a file given ' + + 'as "path-to-additional-verbs" input to your GitHub action ' + + '(currently the file is: /some/path). Please check the ' + + 'whitelist and either change the first word of the subject or ' + + 'whitelist the verb.' + ]); } ); it('accepts the subject starting with an additional verb.', () => { + const inputs = input.parseInputs('table', '', 'false').mustInputs(); + const message = 'Table that for me\n\nThis is a dummy commit.'; - const errors = inspection.check( - message, - new Set(['table']), - false - ); + const errors = inspection.check(message, inputs); expect(errors).toEqual([]); }); @@ -143,15 +228,23 @@ it('reports the subject ending in a dot.', () => { 'This replaces the SomeClass with OtherClass in all of the module \n' + 'since Some class was deprecated.'; - const errors = inspection.check(message, new Set(), false); - expect(errors).toEqual(["The subject must not end with a dot ('.')."]); + const errors = inspection.check(message, defaultInputs); + expect(errors).toEqual([ + "The subject must not end with a dot ('.'). " + + 'Please remove the trailing dot(s).' + ]); }); it('reports an incorrect one-line message with allowed one-liners.', () => { + const inputs = input.parseInputs('', '', 'true').mustInputs(); + const message = 'Change SomeClass to OtherClass.'; - const errors = inspection.check(message, new Set(), true); - expect(errors).toEqual(["The subject must not end with a dot ('.')."]); + const errors = inspection.check(message, inputs); + expect(errors).toEqual([ + "The subject must not end with a dot ('.'). " + + 'Please remove the trailing dot(s).' + ]); }); it('reports too long a body line.', () => { @@ -161,12 +254,13 @@ it('reports too long a body line.', () => { 'This replaces the SomeClass with OtherClass in all of the module ' + 'since Some class was deprecated.'; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([ 'The line 3 of the message (line 1 of the body) exceeds the limit of ' + '72 characters. The line contains 97 characters: ' + '"This replaces the SomeClass with OtherClass in all of the module since ' + - 'Some class was deprecated.".' + 'Some class was deprecated.". ' + + 'Please reformat the body so that all the lines fit 72 characters.' ]); }); @@ -174,7 +268,8 @@ it('accepts a body line of exactly 72 characters.', () => { const message = 'Do something\n' + '\n' + - 'This patch fixes a typo in the readme file where this project was called\n' + + 'This patch fixes a typo in the readme file where this project ' + + 'was called\n' + 'dead-csharp instead of doctest-csharp.\n' + '1234567890' + '1234567890' + @@ -185,7 +280,7 @@ it('accepts a body line of exactly 72 characters.', () => { '1234567890' + '12'; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([]); }); @@ -193,7 +288,8 @@ it('ignores the carriage return.', () => { const message = 'Do something\n' + '\n' + - 'This patch fixes a typo in the readme file where this project was called\r\n' + + 'This patch fixes a typo in the readme file where this project ' + + 'was called\r\n' + 'dead-csharp instead of doctest-csharp.\r\n' + '1234567890' + '1234567890' + @@ -204,14 +300,14 @@ it('ignores the carriage return.', () => { '1234567890' + '12'; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([]); }); it('accepts body that does not start with a word.', () => { const message = 'Change SomeClass to OtherClass\n\n* Do something'; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([]); }); @@ -219,19 +315,23 @@ it('reports duplicate starting word in subject and body.', () => { const message = 'Change SomeClass to OtherClass\n' + '\n' + - 'Change SomeClass so that OtherClass does not conflict..'; + 'Change SomeClass so that OtherClass does not conflict.'; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([ 'The first word of the subject ("Change") must not match ' + - 'the first word of the body.' + 'the first word of the body. ' + + 'Please make the body more informative by adding more information ' + + 'instead of repeating the subject. For example, start by explaining ' + + 'the problem that this change is intended to solve or what was ' + + 'previously missing (e.g., "Previously, ....").' ]); }); it('ignores merge messages.', () => { const message = "Merge branch 'V20DataModel' into miho/Conform-to-spec"; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([]); }); @@ -246,7 +346,7 @@ This patch does something with the URL: ${url} The next line conforms to the line length.`; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([]); }); @@ -261,11 +361,12 @@ it('ignores URL on a separate line, but reports non-conform lines.', () => { This ${long} patch does something with the URL. ${url}`; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([ 'The line 3 of the message (line 1 of the body) exceeds ' + 'the limit of 72 characters. The line contains 92 characters: ' + - `"This ${long} patch does something with the URL.".` + `"This ${long} patch does something with the URL.". ` + + 'Please reformat the body so that all the lines fit 72 characters.' ]); }); @@ -282,7 +383,7 @@ This patch does something with the URL: [1] The next line conforms to the line length.`; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([]); }); @@ -300,10 +401,11 @@ This patch does something with the URL: [1] The ${long} line is too long.`; - const errors = inspection.check(message, new Set(), false); + const errors = inspection.check(message, defaultInputs); expect(errors).toEqual([ 'The line 7 of the message (line 5 of the body) exceeds ' + 'the limit of 72 characters. The line contains 74 characters: ' + - `"The ${long} line is too long.".` + `"The ${long} line is too long.". ` + + 'Please reformat the body so that all the lines fit 72 characters.' ]); }); diff --git a/src/__tests__/main.test.ts b/src/__tests__/main.test.ts index 8527265..95de64e 100644 --- a/src/__tests__/main.test.ts +++ b/src/__tests__/main.test.ts @@ -87,7 +87,7 @@ it('formats properly no error message.', () => { it('formats properly errors on a single message.', () => { (commitMessages.retrieve as any).mockImplementation(() => [ - 'SomeClass to OtherClass\n\nSomeClass with OtherClass' + 'change SomeClass to OtherClass\n\nSomeClass with OtherClass' ]); const mockSetFailed = jest.fn(); @@ -97,9 +97,11 @@ it('formats properly errors on a single message.', () => { expect(mockSetFailed.mock.calls).toEqual([ [ 'The message 1 is invalid:\n' + - '* The subject must start with a capitalized verb (e.g., "Change").\n' + + '* The subject must start with a capitalized word, but ' + + 'the current first word is: "change". ' + + 'Please capitalize to: "Change".\n' + 'The original message was:\n' + - 'SomeClass to OtherClass\n' + + 'change SomeClass to OtherClass\n' + '\n' + 'SomeClass with OtherClass\n' ] @@ -108,7 +110,7 @@ it('formats properly errors on a single message.', () => { it('formats properly errors on two messages.', () => { (commitMessages.retrieve as any).mockImplementation(() => [ - `SomeClass to OtherClass\n\n${'A'.repeat(73)}`, + `change SomeClass to OtherClass\n\nDo something`, 'Change other subject\n\nChange body' ]); @@ -119,23 +121,21 @@ it('formats properly errors on two messages.', () => { expect(mockSetFailed.mock.calls).toEqual([ [ - `${'The message 1 is invalid:\n' + - '* The subject must start with a capitalized verb (e.g., "Change").\n' + - '* The line 3 of the message (line 1 of the body) exceeds ' + - 'the limit of 72 characters. The line contains 73 characters: "'}${'A'.repeat( - 73 - )}".\n` + - `The original message was:\n` + - `SomeClass to OtherClass\n` + - `\n${'A'.repeat(73)}\n` + - `\n` + - `The message 2 is invalid:\n` + - `* The first word of the subject ("Change") must not match ` + - `the first word of the body.\n` + - `The original message was:\n` + - `Change other subject\n` + - `\n` + - `Change body\n` + 'The message 1 is invalid:\n' + + '* The subject must start with a capitalized word, ' + + 'but the current first word is: "change". ' + + 'Please capitalize to: "Change".\n' + + 'The original message was:\n' + + 'change SomeClass to OtherClass\n\nDo something\n\n' + + 'The message 2 is invalid:\n' + + '* The first word of the subject ("Change") must not match ' + + 'the first word of the body. Please make the body more informative ' + + 'by adding more information instead of repeating the subject. ' + + 'For example, start by explaining the problem that this change ' + + 'is intended to solve or what was previously missing ' + + '(e.g., "Previously, ....").\n' + + 'The original message was:\n' + + 'Change other subject\n\nChange body\n' ] ]); }); diff --git a/src/input.ts b/src/input.ts index ba98721..c66d114 100644 --- a/src/input.ts +++ b/src/input.ts @@ -1,3 +1,112 @@ +import fs from 'fs'; + +export class Inputs { + public hasAdditionalVerbsInput: boolean; + public pathToAdditionalVerbs: string; + public allowOneLiners: boolean; + + // This is a complete appendix to the whiltelist parsed both from + // the GitHub action input "additional-verbs" and from the file + // specified by the input "path-to-additional-verbs". + additionalVerbs: Set; + + constructor( + hasAdditionalVerbsInput: boolean, + pathToAdditionalVerbs: string, + allowOneLiners: boolean, + additionalVerbs: Set + ) { + this.hasAdditionalVerbsInput = hasAdditionalVerbsInput; + this.pathToAdditionalVerbs = pathToAdditionalVerbs; + this.allowOneLiners = allowOneLiners; + this.additionalVerbs = additionalVerbs; + } +} + +export class MaybeInputs { + public inputs: Inputs | null; + public error: string | null; + + constructor(inputs: Inputs | null, error: string | null) { + if (inputs === null && error === null) { + throw Error("Unexpected both 'inputs' and 'error' arguments to be null."); + } + + if (inputs !== null && error !== null) { + throw Error( + "Unexpected both 'inputs' and 'error' arguments to be given." + ); + } + + this.inputs = inputs; + this.error = error; + } + + public mustInputs(): Inputs { + if (this.inputs === null) { + throw Error( + "The field 'inputs' is expected to be set, but it is null. " + + `The field 'error' is: ${this.error}` + ); + } + return this.inputs; + } +} + +export function parseInputs( + additionalVerbsInput: string, + pathToAdditionalVerbsInput: string, + allowOneLinersInput: string +): MaybeInputs { + const additionalVerbs = new Set(); + + const hasAdditionalVerbsInput = additionalVerbsInput.length > 0; + + if (additionalVerbsInput) { + for (const verb of parseVerbs(additionalVerbsInput)) { + additionalVerbs.add(verb); + } + } + + if (pathToAdditionalVerbsInput) { + if (!fs.existsSync(pathToAdditionalVerbsInput)) { + return new MaybeInputs( + null, + 'The file referenced by path-to-additional-verbs could ' + + `not be found: ${pathToAdditionalVerbsInput}` + ); + } + + const text = fs.readFileSync(pathToAdditionalVerbsInput).toString('utf-8'); + + for (const verb of parseVerbs(text)) { + additionalVerbs.add(verb); + } + } + + const allowOneLiners: boolean | null = !allowOneLinersInput + ? false + : parseAllowOneLiners(allowOneLinersInput); + + if (allowOneLiners === null) { + return new MaybeInputs( + null, + 'Unexpected value for allow-one-liners. ' + + `Expected either 'true' or 'false', got: ${allowOneLinersInput}` + ); + } + + return new MaybeInputs( + new Inputs( + hasAdditionalVerbsInput, + pathToAdditionalVerbsInput, + allowOneLiners, + additionalVerbs + ), + null + ); +} + export function parseVerbs(text: string): string[] { const lines = text.split('\n'); diff --git a/src/inspection.ts b/src/inspection.ts index 653f381..cb577fb 100644 --- a/src/inspection.ts +++ b/src/inspection.ts @@ -1,4 +1,5 @@ import * as mostFrequentEnglishVerbs from './mostFrequentEnglishVerbs'; +import * as input from './input'; interface SubjectBody { subject: string; @@ -48,13 +49,90 @@ function splitSubjectBody(lines: string[]): MaybeSubjectBody { return result; } -const capitalizedWordRe = new RegExp('^([A-Z][a-z]*)[^a-zA-Z]'); - +const allLettersRe = new RegExp('^[a-zA-Z][a-zA-Z-]+$'); +const firstWordBeforeSpaceRe = new RegExp('^([a-zA-Z][a-zA-Z-]+)\\s'); const suffixHashCodeRe = new RegExp('\\s?\\(\\s*#[a-zA-Z_0-9]+\\s*\\)$'); -function checkSubject(subject: string, additionalVerbs: Set): string[] { +function extractFirstWord(text: string): string | null { + if (text.length === 0) { + return null; + } + + if (text.match(allLettersRe)) { + return text; + } else { + const match = firstWordBeforeSpaceRe.exec(text); + if (!match) { + return null; + } + + return match[1]; + } +} + +function capitalize(word: string): string { + if (word.length === 0) { + return ''; + } else if (word.length === 1) { + return word.toUpperCase(); + } else { + return word[0].toUpperCase() + word.slice(1).toLowerCase(); + } +} + +function errorMessageOnNonVerb( + firstWord: string, + inputs: input.Inputs +): string { + const parts = [ + 'The subject must start with a verb in imperative mood, ' + + `but it started with: ${JSON.stringify(firstWord)}. ` + + 'Whether the word is in imperative mood is determined by ' + + 'whitelisting. The general whitelist is available at ' + + 'https://github.com/mristin/opinionated-commit-message/' + + 'blob/master/src/mostFrequentEnglishVerbs.ts.' + ]; + + if (!inputs.hasAdditionalVerbsInput) { + parts.push( + 'You can whitelist additional verbs using ' + + '"additional-verbs" input to your GitHub action ' + + '(currently no additional verbs were thus specified).' + ); + } else { + parts.push( + 'You can whitelist additional verbs using ' + + '"additional-verbs" input to your GitHub action ' + + `(currently one or more additional verbs were thus ` + + 'specified).' + ); + } + + if (inputs.pathToAdditionalVerbs.length === 0) { + parts.push( + 'Moreover, you can also whitelist additional verbs in a file ' + + 'given as "path-to-additional-verbs" input to your GitHub action ' + + '(currently no whitelist file was specified).' + ); + } else { + parts.push( + 'Moreover, you can also whitelist additional verbs in a file ' + + 'given as "path-to-additional-verbs" input to your GitHub action ' + + `(currently the file is: ${inputs.pathToAdditionalVerbs}).` + ); + } + + parts.push( + 'Please check the whitelist and either change the first word ' + + 'of the subject or whitelist the verb.' + ); + + return parts.join(' '); +} + +function checkSubject(subject: string, inputs: input.Inputs): string[] { // Pre-condition - for (const verb of additionalVerbs) { + for (const verb of inputs.additionalVerbs) { if (verb.length === 0) { throw new Error(`Unexpected empty additional verb`); } @@ -76,54 +154,53 @@ function checkSubject(subject: string, additionalVerbs: Set): string[] { if (subjectWoCode.length > 50) { errors.push( `The subject exceeds the limit of 50 characters ` + - `(got: ${subject.length}, JSONified: ${JSON.stringify(subjectWoCode)})` + `(got: ${subject.length}, JSON: ${JSON.stringify(subjectWoCode)}).` + + 'Please shorten the subject to make it more succinct.' ); } - const match = capitalizedWordRe.exec(subjectWoCode); - - if (!match) { + const firstWord = extractFirstWord(subjectWoCode); + if (!firstWord) { errors.push( - 'The subject must start with a capitalized verb (e.g., "Change").' + 'Expected the subject to start with a verb in imperative mood ' + + 'consisting of letters and possibly dashes in-between, ' + + `but the subject was: ${JSON.stringify(subjectWoCode)}. ` + + 'Please re-write the subject so that it starts with ' + + 'a verb in imperative mood.' ); } else { - if (match.length < 2) { - throw Error( - 'Expected at least one group to match the first capitalized word, ' + - 'but got none.' + const capitalized = capitalize(firstWord); + if (firstWord !== capitalized) { + errors.push( + 'The subject must start with a capitalized word, ' + + `but the current first word is: ${JSON.stringify(firstWord)}. ` + + `Please capitalize to: ${JSON.stringify(capitalized)}.` ); } - const word = match[1]; if ( - !mostFrequentEnglishVerbs.SET.has(word.toLowerCase()) && - !additionalVerbs.has(word.toLowerCase()) + !mostFrequentEnglishVerbs.SET.has(firstWord.toLowerCase()) && + !inputs.additionalVerbs.has(firstWord.toLowerCase()) ) { - if (additionalVerbs.size === 0) { - errors.push( - 'The subject must start in imperative mood with one of the ' + - `most frequent English verbs, but got: ${JSON.stringify(word)}. ` + - 'Please see ' + - 'https://github.com/mristin/opinionated-commit-message/blob/master/' + - 'src/mostFrequentEnglishVerbs.ts ' + - 'for a complete list.' - ); - } else { - errors.push( - 'The subject must start in imperative mood with one of the ' + - `most frequent English verbs, but got: ${JSON.stringify(word)}. ` + - 'Please see ' + - 'https://github.com/mristin/opinionated-commit-message/blob/master/' + - 'src/mostFrequentEnglishVerbs.ts ' + - 'for a complete list and ' + - 'also revisit your list of additional verbs.' - ); - } + /* + (mristin, 2020-09-09): It might be worthwhile to refactor the rendering + of the error messages to a separate module and use classes to represent + the errors. The complexity is still manageable, so it is not yet the + moment to do so since the refactoring would be quite time-consuming. + + Originally, I did not foresee that error messages need such a large + information content. + */ + + errors.push(errorMessageOnNonVerb(firstWord, inputs)); } } if (subjectWoCode.endsWith('.')) { - errors.push("The subject must not end with a dot ('.')."); + errors.push( + "The subject must not end with a dot ('.'). " + + 'Please remove the trailing dot(s).' + ); } return errors; @@ -157,29 +234,29 @@ function checkBody(subject: string, bodyLines: string[]): string[] { `The line ${i + 3} of the message (line ${i + 1} of the body) ` + 'exceeds the limit of 72 characters. ' + `The line contains ${line.length} characters: ` + - `${JSON.stringify(line)}.` + `${JSON.stringify(line)}. ` + + 'Please reformat the body so that all the lines fit ' + + '72 characters.' ); } } - const bodyFirstWordMatch = capitalizedWordRe.exec(bodyLines[0]); + const bodyFirstWord = extractFirstWord(bodyLines[0]); - if (bodyFirstWordMatch) { - const bodyFirstWord = bodyFirstWordMatch[1]; + if (bodyFirstWord) { + const subjectFirstWord = extractFirstWord(subject); - const subjectFirstWordMatch = capitalizedWordRe.exec(subject); - if ( - subjectFirstWordMatch !== undefined && - subjectFirstWordMatch !== null && - subjectFirstWordMatch.length > 0 - ) { - const subjectFirstWord = subjectFirstWordMatch[1]; + if (subjectFirstWord) { if (subjectFirstWord.toLowerCase() === bodyFirstWord.toLowerCase()) { errors.push( 'The first word of the subject ' + `(${JSON.stringify(subjectFirstWord)}) ` + - 'must not match ' + - 'the first word of the body.' + 'must not match the first word of the body. ' + + 'Please make the body more informative by adding more ' + + 'information instead of repeating the subject. ' + + 'For example, start by explaining the problem that this change ' + + 'is intended to solve or what was previously missing ' + + '(e.g., "Previously, ....").' ); } } @@ -193,11 +270,7 @@ const mergeMessageRe = new RegExp( 'into [^\\000-\\037\\177 ~^:?*[]+$' ); -export function check( - message: string, - additionalVerbs: Set, - allowOneLiners: boolean -): string[] { +export function check(message: string, inputs: input.Inputs): string[] { const errors: string[] = []; if (mergeMessageRe.test(message)) { @@ -209,8 +282,8 @@ export function check( if (lines.length === 0) { errors.push(`The message is empty.`); return errors; - } else if (lines.length === 1 && allowOneLiners) { - errors.push(...checkSubject(lines[0], additionalVerbs)); + } else if (lines.length === 1 && inputs.allowOneLiners) { + errors.push(...checkSubject(lines[0], inputs)); } else { const maybeSubjectBody = splitSubjectBody(lines); if (maybeSubjectBody.errors.length > 0) { @@ -221,7 +294,7 @@ export function check( } const subjectBody = maybeSubjectBody.subjectBody; - errors.push(...checkSubject(subjectBody.subject, additionalVerbs)); + errors.push(...checkSubject(subjectBody.subject, inputs)); errors.push(...checkBody(subjectBody.subject, subjectBody.bodyLines)); } diff --git a/src/mainImpl.ts b/src/mainImpl.ts index c1effc9..5d5ba22 100644 --- a/src/mainImpl.ts +++ b/src/mainImpl.ts @@ -1,5 +1,3 @@ -import fs from 'fs'; - import * as core from '@actions/core'; import * as commitMessages from './commitMessages'; @@ -10,66 +8,42 @@ import * as input from './input'; function runWithExceptions(): void { const messages: string[] = commitMessages.retrieve(); - const additionalVerbs = new Set(); - - // Parse additional-verbs input + //// + // Parse inputs + //// - const additionalVerbsInput = core.getInput('additional-verbs', { - required: false - }); + const additionalVerbsInput = + core.getInput('additional-verbs', {required: false}) ?? ''; - if (additionalVerbsInput) { - for (const verb of input.parseVerbs(additionalVerbsInput)) { - additionalVerbs.add(verb); - } - } + const pathToAdditionalVerbsInput = + core.getInput('path-to-additional-verbs', {required: false}) ?? ''; - // Parse additional-verbs-from-path input - - const pathToAdditionalVerbs = core.getInput('path-to-additional-verbs', { - required: false - }); - - if (pathToAdditionalVerbs) { - if (!fs.existsSync(pathToAdditionalVerbs)) { - const error = - 'The file referenced by path-to-additional-verbs could ' + - `not be found: ${pathToAdditionalVerbs}`; - core.error(error); - core.setFailed(error); - return; - } + const allowOneLinersInput = + core.getInput('allow-one-liners', {required: false}) ?? ''; - const text = fs.readFileSync(pathToAdditionalVerbs).toString('utf-8'); + const maybeInputs = input.parseInputs( + additionalVerbsInput, + pathToAdditionalVerbsInput, + allowOneLinersInput + ); - for (const verb of input.parseVerbs(text)) { - additionalVerbs.add(verb); - } + if (maybeInputs.error !== null) { + core.error(maybeInputs.error); + core.setFailed(maybeInputs.error); + return; } - // Parse allow-one-liners input - const allowOneLinersText = core.getInput('allow-one-liners', { - required: false - }); - - const allowOneLiners: boolean | null = !allowOneLinersText - ? false - : input.parseAllowOneLiners(allowOneLinersText); + const inputs = maybeInputs.mustInputs(); - if (allowOneLiners === null) { - const error = - 'Unexpected value for allow-one-liners. ' + - `Expected either 'true' or 'false', got: ${allowOneLinersText}`; - core.error(error); - core.setFailed(error); - return; - } + //// + // Inspect + //// // Parts of the error message to be concatenated with '\n' const parts: string[] = []; for (const [messageIndex, message] of messages.entries()) { - const errors = inspection.check(message, additionalVerbs, allowOneLiners); + const errors = inspection.check(message, inputs); if (errors.length > 0) { const repr: string = represent.formatErrors( diff --git a/src/mostFrequentEnglishVerbs.ts b/src/mostFrequentEnglishVerbs.ts index 1046ac8..79e617b 100644 --- a/src/mostFrequentEnglishVerbs.ts +++ b/src/mostFrequentEnglishVerbs.ts @@ -733,5 +733,6 @@ export const SET = new Set([ 'reinstate', 'pin', 'hint', - 'integrate' + 'integrate', + 'instruct' ]);