diff --git a/docs/manual/conventional-commits.md b/docs/manual/conventional-commits.md index a6147de7..048de022 100644 --- a/docs/manual/conventional-commits.md +++ b/docs/manual/conventional-commits.md @@ -21,12 +21,12 @@ Here is the full syntax for a commit message: [optional footer] ``` -The standad says that description is meant to start with a _lowercase letter_, so this is applied throughout this project. +The standard says that description is meant to start with a _lowercase letter_, so this is applied throughout this project. This project focuses only on the first part: ``` -(SCOPE): MESSAGE +(SCOPE): FILE_CHANGE_DESCRIPTION ``` diff --git a/src/prepareCommitMsg.ts b/src/prepareCommitMsg.ts index ad5be74f..2640f266 100644 --- a/src/prepareCommitMsg.ts +++ b/src/prepareCommitMsg.ts @@ -4,6 +4,9 @@ * This module ties together logic from modules in the `generate` module. So it is best kept outside * that. * + * The "message" is the full commit message. The "file change description" is the description + * portion, which describes how the files changed. + * * This module doesn't interact with the git CLI or the extension. It just deals with text. */ import { lookupDiffIndexAction } from "./generate/action"; @@ -13,6 +16,36 @@ import { parseDiffIndex } from "./git/parseOutput"; import { CONVENTIONAL_TYPE } from "./lib/constants"; import { equal } from "./lib/utils"; +/** + * Join two strings together with a space, or just one string if only one is set. + * + * Trimming on the outside is necessary, in case only one item is set. + * Don't both trimming the items before joining them, as this project doesn't need that for where + * this is used. + */ +export function _cleanJoin(first: string, second: string) { + return [first, second].join(" ").trim(); +} + +/** + * Separate a message into a Conventional Commit prefix, if any, and the description. + * + * Require a colon to exist to detect prefix. i.e. 'ci' will be considered a description, but 'ci:' + * will be considered a prefix. This keeps the check simpler as we don't have to match against every + * type and we don' have to check if we are part of a word e.g. 'circus'. + */ +export function _splitMsg(msg: string) { + const [prefix, fileChangeDesc] = msg.includes(":") + ? msg.split(":") + : ["", msg]; + + const [customPrefix, typePrefix] = prefix.includes(" ") + ? prefix.split(" ", 2) + : ["", prefix]; + + return { customPrefix, typePrefix, fileChangeDesc: fileChangeDesc.trim() }; +} + /** * Determine what the prefix should be for a file change, using Conventional Commit standard. */ @@ -31,9 +64,9 @@ export function _msgOne(line: string) { // Don't unpack as {x, y, from, to} // const fileChanges = parseDiffIndex(line) const prefix = _prefixFromChanges(line), - fileChangeMsg = oneChange(line); + fileChangeDesc = oneChange(line); - return { prefix, fileChangeMsg }; + return { prefix, fileChangeDesc }; } /** @@ -67,7 +100,7 @@ export function _msgMulti(lines: string[]) { const conventions = lines.map(_prefixFromChanges); const convention = collapse(conventions); - return { prefix: convention, fileChangeMsg: namedFiles(lines) }; + return { prefix: convention, fileChangeDesc: namedFiles(lines) }; } /** @@ -87,42 +120,64 @@ export function _msgFromChanges(diffIndexLines: string[]) { /** * Output a readable conventional commit message. */ -export function _formatMsg(prefix: CONVENTIONAL_TYPE, fileChangeMsg: string) { +export function _formatMsg(prefix: CONVENTIONAL_TYPE, fileChangeDesc: string) { if (prefix === CONVENTIONAL_TYPE.UNKNOWN) { - return fileChangeMsg; + return fileChangeDesc; } - return `${prefix}: ${fileChangeMsg}`; + return `${prefix}: ${fileChangeDesc}`; } /** * Generate a new commit message and format it as a string. */ export function _newMsg(lines: string[]) { - const { prefix, fileChangeMsg } = _msgFromChanges(lines); + const { prefix, fileChangeDesc } = _msgFromChanges(lines); - return _formatMsg(prefix, fileChangeMsg); + return _formatMsg(prefix, fileChangeDesc); } /** * Format commit message using old and new messages. * - * For now, assume old message is a commit message template prefix and can always go in front, - * removing any existing twice on either side for flexibility. - * - * Dev note - must make sure prefix and fileChangeMsg come in separately here, not as a combined - * message. + * The point is to always use the new message and prefix, but respect the old message. If there is + * no new prefix type set, use the old one, and if that is not set then just a simple message will + * do. * - * TODO: Check if the old message is already a PREFIX form or a PREFIX FILECHANGE form. This changes - * the new message form. + * @param prefix The Conventional Commit prefix to use, as generated by the extension based on + * changed files. + * @param fileChangeDesc A description of changes, also generated. + * @param oldMsg What exists in the commit message box when the extension is run, whether typed or + * generated previously. */ export function _combineOldAndNew( prefix: CONVENTIONAL_TYPE, - fileChangeMsg: string, + fileChangeDesc: string, oldMsg?: string ) { - const newMsg = _formatMsg(prefix, fileChangeMsg); + if (!oldMsg) { + return _formatMsg(prefix, fileChangeDesc); + } + + const { + customPrefix: oldCustomPrefix, + typePrefix: oldTypePrefix, + fileChangeDesc: oldFileChangeDesc, + } = _splitMsg(oldMsg); + + const fileChangeDescResult = _cleanJoin(oldFileChangeDesc, fileChangeDesc); + + if (prefix !== CONVENTIONAL_TYPE.UNKNOWN) { + return `${_cleanJoin(oldCustomPrefix, prefix)}: ${fileChangeDescResult}`; + } + + if (oldTypePrefix) { + return `${_cleanJoin( + oldCustomPrefix, + oldTypePrefix + )}: ${fileChangeDescResult}`; + } - return oldMsg ? `${oldMsg.trim()} ${newMsg}` : newMsg; + return fileChangeDescResult; } /** @@ -137,9 +192,9 @@ function generateMsgWithOld(fileChanges: string[], oldMsg: string) { "Either `oldMsg` must not be empty, or use `generateNewMsg` instead." ); } - const { prefix, fileChangeMsg } = _msgFromChanges(fileChanges); + const { prefix, fileChangeDesc } = _msgFromChanges(fileChanges); - return _combineOldAndNew(prefix, fileChangeMsg, oldMsg); + return _combineOldAndNew(prefix, fileChangeDesc, oldMsg); } /** diff --git a/src/test/prepareCommitMsg.test.ts b/src/test/prepareCommitMsg.test.ts index adfc4be3..8cacb9f1 100644 --- a/src/test/prepareCommitMsg.test.ts +++ b/src/test/prepareCommitMsg.test.ts @@ -6,18 +6,99 @@ import * as assert from "assert"; import { CONVENTIONAL_TYPE } from "../lib/constants"; import { + _cleanJoin, _combineOldAndNew, _formatMsg, _msgFromChanges, _newMsg, + _splitMsg, } from "../prepareCommitMsg"; +describe("Join strings cleanly", function () { + describe("#_cleanJoin", function () { + it("joins two set strings", function () { + assert.strictEqual(_cleanJoin("abc", "def"), "abc def"); + assert.strictEqual(_cleanJoin(" abc", "def "), "abc def"); + }); + + it("uses the first string if the second is not set", function () { + assert.strictEqual(_cleanJoin("abc", ""), "abc"); + assert.strictEqual(_cleanJoin("abc ", ""), "abc"); + }); + + it("uses the second string if the first is not set", function () { + assert.strictEqual(_cleanJoin("", "abc def"), "abc def"); + assert.strictEqual(_cleanJoin("", "abc def "), "abc def"); + }); + }); +}); + +describe("Split a message into components", function () { + describe("#_splitMsg", function () { + it("handles a description alone", function () { + assert.deepStrictEqual(_splitMsg("abc def"), { + customPrefix: "", + typePrefix: "", + fileChangeDesc: "abc def", + }); + assert.deepStrictEqual(_splitMsg("[ABCD-1234]"), { + customPrefix: "", + typePrefix: "", + fileChangeDesc: "[ABCD-1234]", + }); + }); + + it("handles a prefix alone", function () { + assert.deepStrictEqual(_splitMsg("docs:"), { + customPrefix: "", + typePrefix: "docs", + fileChangeDesc: "", + }); + assert.deepStrictEqual(_splitMsg("feat:"), { + customPrefix: "", + typePrefix: "feat", + fileChangeDesc: "", + }); + + assert.deepStrictEqual(_splitMsg("docs: "), { + customPrefix: "", + typePrefix: "docs", + fileChangeDesc: "", + }); + }); + + it("separates a prefix and description", function () { + assert.deepStrictEqual(_splitMsg("docs: abc"), { + customPrefix: "", + typePrefix: "docs", + fileChangeDesc: "abc", + }); + assert.deepStrictEqual(_splitMsg("docs: abc def"), { + customPrefix: "", + typePrefix: "docs", + fileChangeDesc: "abc def", + }); + assert.deepStrictEqual(_splitMsg("feat: abc def"), { + customPrefix: "", + typePrefix: "feat", + fileChangeDesc: "abc def", + }); + + assert.deepStrictEqual(_splitMsg("[ABCD-1234] docs: abc def"), { + customPrefix: "[ABCD-1234]", + typePrefix: "docs", + fileChangeDesc: "abc def", + }); + }); + }); +}); + describe("Prepare commit message", function () { describe("#_msgFromChanges", function () { it("handles a single file correctly", function () { const expected = { prefix: CONVENTIONAL_TYPE.FEAT, - fileChangeMsg: "create baz.txt", + fileChangeDesc: "create baz.txt", }; assert.deepStrictEqual(_msgFromChanges(["A baz.txt"]), expected); @@ -27,7 +108,7 @@ describe("Prepare commit message", function () { it("handles 2 created files created correctly", function () { const expected = { prefix: CONVENTIONAL_TYPE.FEAT, - fileChangeMsg: "create baz.txt and bar.js", + fileChangeDesc: "create baz.txt and bar.js", }; assert.deepStrictEqual( @@ -39,7 +120,7 @@ describe("Prepare commit message", function () { it("handles 2 modified files correctly", function () { const expected = { prefix: CONVENTIONAL_TYPE.UNKNOWN, - fileChangeMsg: "update baz.txt and bar.js", + fileChangeDesc: "update baz.txt and bar.js", }; assert.deepStrictEqual( @@ -51,7 +132,7 @@ describe("Prepare commit message", function () { it("handles 3 files with the same action correctly", function () { const expected = { prefix: CONVENTIONAL_TYPE.FEAT, - fileChangeMsg: "create baz.txt, bar.js and fuzz.md", + fileChangeDesc: "create baz.txt, bar.js and fuzz.md", }; assert.deepStrictEqual( @@ -63,7 +144,7 @@ describe("Prepare commit message", function () { it("handles 4 files with the same action correctly", function () { const expected = { prefix: CONVENTIONAL_TYPE.FEAT, - fileChangeMsg: "create baz.txt, bar.js, fuzz.md and fuzz.ts", + fileChangeDesc: "create baz.txt, bar.js, fuzz.md and fuzz.ts", }; assert.deepStrictEqual( @@ -80,7 +161,7 @@ describe("Prepare commit message", function () { it("handles 3 files in subdirectories but does not show the directory paths", function () { const expected = { prefix: CONVENTIONAL_TYPE.FEAT, - fileChangeMsg: "create baz.txt, bar.js and fuzz.md", + fileChangeDesc: "create baz.txt, bar.js and fuzz.md", }; assert.deepStrictEqual( @@ -97,7 +178,7 @@ describe("Prepare commit message", function () { it('handles 2 "build(deps)" files correctly', function () { const expected = { prefix: CONVENTIONAL_TYPE.BUILD_DEPENDENCIES, - fileChangeMsg: "update package.json and package-lock.json", + fileChangeDesc: "update package.json and package-lock.json", }; assert.deepStrictEqual( @@ -109,7 +190,7 @@ describe("Prepare commit message", function () { it("handles 3 README.md files in different locations as full paths", function () { const expected = { prefix: CONVENTIONAL_TYPE.DOCS, - fileChangeMsg: "update docs/README.md, bar/README.md and README.md", + fileChangeDesc: "update docs/README.md, bar/README.md and README.md", }; assert.deepStrictEqual( @@ -127,7 +208,7 @@ describe("Prepare commit message", function () { it("handles 2 files - one created and one modified", function () { const expected = { prefix: CONVENTIONAL_TYPE.UNKNOWN, - fileChangeMsg: "Various changes to baz.txt and bar.js", + fileChangeDesc: "Various changes to baz.txt and bar.js", }; assert.deepStrictEqual( @@ -139,7 +220,7 @@ describe("Prepare commit message", function () { it("handles 3 files - with different actions", function () { const expected = { prefix: CONVENTIONAL_TYPE.UNKNOWN, - fileChangeMsg: "Various changes to baz.txt, bar.js and README.md", + fileChangeDesc: "Various changes to baz.txt, bar.js and README.md", }; assert.deepStrictEqual( @@ -176,7 +257,7 @@ describe("Prepare commit message", function () { }); it("handles multiple changes", function () { - // Leave the detailed cases to tests for _msgFromChanges. + // Leave the detailed cases to tests for `_msgFromChanges`. assert.strictEqual( _newMsg(["A baz.txt", "A bar.js"]), @@ -201,42 +282,127 @@ describe("Prepare commit message", function () { }); describe("#_combineOldAndNew", function () { - it("combines uses the new message if there is no old message", function () { + it("uses only the new message, if the old message is empty", function () { assert.strictEqual( - _combineOldAndNew(CONVENTIONAL_TYPE.FEAT, "Foo bar"), - "feat: Foo bar" + _combineOldAndNew(CONVENTIONAL_TYPE.UNKNOWN, "foo bar"), + "foo bar" ); assert.strictEqual( - _combineOldAndNew(CONVENTIONAL_TYPE.FEAT, "Foo bar", ""), - "feat: Foo bar" + _combineOldAndNew(CONVENTIONAL_TYPE.FEAT, "foo bar"), + "feat: foo bar" ); - }); - - it("combines an existing message and a new message", function () { - // Typical case is '[JIRA_TICKET] docs:' has 'Update foo' added. - // Though this ends up duplicating docs and feat possible. - // This isn't so smart yet but helps sometimes. assert.strictEqual( - _combineOldAndNew(CONVENTIONAL_TYPE.UNKNOWN, "Foo bar", "Fizz buzz"), - "Fizz buzz Foo bar" + _combineOldAndNew(CONVENTIONAL_TYPE.FEAT, "foo bar", ""), + "feat: foo bar" ); + }); - assert.strictEqual( - _combineOldAndNew(CONVENTIONAL_TYPE.UNKNOWN, "Foo bar", "feat:"), - "feat: Foo bar" - ); - assert.strictEqual( - _combineOldAndNew(CONVENTIONAL_TYPE.UNKNOWN, "Foo bar", "feat: "), - "feat: Foo bar" - ); + describe("combines an existing message with a new message that is set", function () { + // Using '[ABCD-1234]' as a Jira ticket number. A branch or project name works too. + + describe("when convention cannot be determined from the file changes", function () { + it("combines two plain messages", function () { + assert.strictEqual( + _combineOldAndNew( + CONVENTIONAL_TYPE.UNKNOWN, + "foo bar", + "fizz buzz" + ), + "fizz buzz foo bar" + ); + + assert.strictEqual( + _combineOldAndNew( + CONVENTIONAL_TYPE.UNKNOWN, + "foo bar", + "[ABCD-1234]" + ), + "[ABCD-1234] foo bar" + ); + }); + + it("combines a plain message and an existing prefix", function () { + assert.strictEqual( + _combineOldAndNew(CONVENTIONAL_TYPE.UNKNOWN, "foo bar", "feat:"), + "feat: foo bar" + ); + + assert.strictEqual( + _combineOldAndNew( + CONVENTIONAL_TYPE.UNKNOWN, + "foo bar", + "[ABCD-1234] feat:" + ), + "[ABCD-1234] feat: foo bar" + ); + }); + + it("combines a plain message and an existing prefix with a space after it", function () { + assert.strictEqual( + _combineOldAndNew(CONVENTIONAL_TYPE.UNKNOWN, "foo bar", "feat: "), + "feat: foo bar" + ); + + assert.strictEqual( + _combineOldAndNew( + CONVENTIONAL_TYPE.UNKNOWN, + "foo bar", + "[ABCD-1234] feat: " + ), + "[ABCD-1234] feat: foo bar" + ); + }); + }); - // This isn't intended but currently how it works. - assert.strictEqual( - _combineOldAndNew(CONVENTIONAL_TYPE.FEAT, "Foo bar", "Fizz buzz"), - "Fizz buzz feat: Foo bar" - ); + describe("when a convention is determined from the file changes", function () { + it("inserts a new prefix between the old and new messages", function () { + assert.strictEqual( + _combineOldAndNew(CONVENTIONAL_TYPE.FEAT, "foo bar", "fizz buzz"), + "feat: fizz buzz foo bar" + ); + + // Unfortunately if your old message doesn't look like a prefix by having a colon, it just + // gets treated as an old description and not something to add before the type. + assert.strictEqual( + _combineOldAndNew(CONVENTIONAL_TYPE.FEAT, "foo bar", "[ABCD-1234]"), + "feat: [ABCD-1234] foo bar" + ); + }); + + it("inserts replaces an old prefix with a new one", function () { + assert.strictEqual( + _combineOldAndNew(CONVENTIONAL_TYPE.FEAT, "foo bar", "docs:"), + "feat: foo bar" + ); + + assert.strictEqual( + _combineOldAndNew( + CONVENTIONAL_TYPE.FEAT, + "foo bar", + "[ABCD-1234] docs:" + ), + "[ABCD-1234] feat: foo bar" + ); + }); + + it("inserts replaces an old prefix with a space with a new one", function () { + assert.strictEqual( + _combineOldAndNew(CONVENTIONAL_TYPE.FEAT, "foo bar", "docs: "), + "feat: foo bar" + ); + + assert.strictEqual( + _combineOldAndNew( + CONVENTIONAL_TYPE.FEAT, + "foo bar", + "[ABCD-1234] docs: " + ), + "[ABCD-1234] feat: foo bar" + ); + }); + }); }); }); });