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' ]);