diff --git a/commitlint.config.ts b/commitlint.config.ts index eec5eb87..96ab0892 100644 --- a/commitlint.config.ts +++ b/commitlint.config.ts @@ -6,8 +6,19 @@ let bodyMaxLineLength = 64; let headerMaxLineLength = 50; let footerMaxLineLength = 150; -function notNullStringErrorMessage(stringType: string): string { - return `This is unexpected because ${stringType} should never be null`; +function notStringErrorMessage(variableName: string): string { + return `This is unexpected because ${variableName} should have been a string`; +} + +function extractStringFromCommitlintParam( + paramName: string, + variable: any +): string { + let str = Helpers.assertNotNone( + Helpers.convertAnyToString(variable), + notStringErrorMessage(paramName) + ); + return str; } export default { @@ -64,27 +75,21 @@ export default { { rules: { "body-prose": ({ raw }: { raw: any }) => { - let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw, "raw"), - notNullStringErrorMessage("raw") - ); + let rawStr = extractStringFromCommitlintParam("raw", raw); return Plugins.bodyProse(rawStr); }, "commit-hash-alone": ({ raw }: { raw: any }) => { - let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw, "raw"), - notNullStringErrorMessage("raw") - ); + let rawStr = extractStringFromCommitlintParam("raw", raw); return Plugins.commitHashAlone(rawStr); }, "empty-wip": ({ header }: { header: any }) => { - let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), - notNullStringErrorMessage("header") + let headerStr = extractStringFromCommitlintParam( + "header", + header ); return Plugins.emptyWip(headerStr); @@ -95,9 +100,9 @@ export default { _: any, maxLineLength: number ) => { - let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), - notNullStringErrorMessage("header") + let headerStr = extractStringFromCommitlintParam( + "header", + header ); return Plugins.headerMaxLengthWithSuggestions( @@ -107,15 +112,12 @@ export default { }, "footer-notes-misplacement": ({ body }: { body: any }) => { - let bodyStr = Helpers.convertAnyToString(body, "body"); - return Plugins.footerNotesMisplacement(bodyStr); + let maybeBody = Helpers.convertAnyToString(body); + return Plugins.footerNotesMisplacement(maybeBody); }, "footer-refs-validity": ({ raw }: { raw: any }) => { - let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw, "raw"), - notNullStringErrorMessage("raw") - ); + let rawStr = extractStringFromCommitlintParam("raw", raw); return Plugins.footerRefsValidity(rawStr); }, @@ -125,54 +127,48 @@ export default { }: { header: any; }) => { - let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), - notNullStringErrorMessage("header") + let headerStr = extractStringFromCommitlintParam( + "header", + header ); return Plugins.preferSlashOverBackslash(headerStr); }, "proper-issue-refs": ({ raw }: { raw: any }) => { - let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw, "raw"), - notNullStringErrorMessage("raw") - ); + let rawStr = extractStringFromCommitlintParam("raw", raw); return Plugins.properIssueRefs(rawStr); }, "title-uppercase": ({ header }: { header: any }) => { - let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), - notNullStringErrorMessage("header") + let headerStr = extractStringFromCommitlintParam( + "header", + header ); return Plugins.titleUppercase(headerStr); }, "too-many-spaces": ({ raw }: { raw: any }) => { - let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw, "raw"), - notNullStringErrorMessage("raw") - ); + let rawStr = extractStringFromCommitlintParam("raw", raw); return Plugins.tooManySpaces(rawStr); }, "type-space-after-colon": ({ header }: { header: any }) => { - let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), - notNullStringErrorMessage("header") + let headerStr = extractStringFromCommitlintParam( + "header", + header ); return Plugins.typeSpaceAfterColon(headerStr); }, "type-with-square-brackets": ({ header }: { header: any }) => { - let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), - notNullStringErrorMessage("header") + let headerStr = extractStringFromCommitlintParam( + "header", + header ); return Plugins.typeWithSquareBrackets(headerStr); @@ -180,17 +176,18 @@ export default { // NOTE: we use 'header' instead of 'subject' as a workaround to this bug: https://github.com/conventional-changelog/commitlint/issues/3404 "subject-lowercase": ({ header }: { header: any }) => { - let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), - notNullStringErrorMessage("header") + let headerStr = extractStringFromCommitlintParam( + "header", + header ); + return Plugins.subjectLowercase(headerStr); }, "type-space-after-comma": ({ header }: { header: any }) => { - let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), - notNullStringErrorMessage("header") + let headerStr = Helpers.assertNotNone( + Helpers.convertAnyToString(header), + notStringErrorMessage("header") ); return Plugins.typeSpaceAfterComma(headerStr); @@ -201,35 +198,32 @@ export default { _: any, maxLineLength: number ) => { - let bodyStr = Helpers.convertAnyToString(body, "body"); + let maybeBody = Helpers.convertAnyToString(body); return Plugins.bodySoftMaxLineLength( - bodyStr, + maybeBody, maxLineLength ); }, "body-paragraph-line-min-length": ({ body }: { body: any }) => { - let bodyStr = Helpers.convertAnyToString(body, "body"); + let maybeBody = Helpers.convertAnyToString(body); return Plugins.bodyParagraphLineMinLength( - bodyStr, + maybeBody, headerMaxLineLength, bodyMaxLineLength ); }, "trailing-whitespace": ({ raw }: { raw: any }) => { - let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw, "raw"), - notNullStringErrorMessage("raw") - ); + let rawStr = extractStringFromCommitlintParam("raw", raw); return Plugins.trailingWhitespace(rawStr); }, "type-space-before-paren": ({ header }: { header: any }) => { - let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), - notNullStringErrorMessage("header") + let headerStr = extractStringFromCommitlintParam( + "header", + header ); return Plugins.typeSpaceBeforeParen(headerStr); diff --git a/commitlint/fpHelpers.ts b/commitlint/fpHelpers.ts new file mode 100644 index 00000000..3b7de9c5 --- /dev/null +++ b/commitlint/fpHelpers.ts @@ -0,0 +1,79 @@ +interface IOption { + /** + * @deprecated it is better to use `if (foo instanceof None)` so that you can access the .value in the `else` case + **/ + IsNone(): boolean; + /** + * @deprecated it is better to use `if (!(foo instanceof None))` so that you can access the .value inside the `if` block + **/ + IsSome(): boolean; +} + +export class None { + public IsNone(): boolean { + return true; + } + public IsSome(): boolean { + return false; + } + + /** + * @deprecated it is better to use `OptionStatic.None` + **/ + constructor() {} +} +export class Some { + value: T; + + constructor(val: NonNullable) { + this.value = val; + } + + public IsNone(): boolean { + return false; + } + public IsSome(): boolean { + return true; + } +} + +export type Option = (None | Some>) & IOption; + +export class OptionStatic { + public static None = new None(); + public static OfObj(obj: T | null | undefined): Option> { + if (obj === null || obj === undefined) { + return OptionStatic.None; + } else { + return new Some(obj); + } + } +} + +export class TypeHelpers { + public static IsNullOrUndefined(variable: any) { + return variable === null || variable === undefined; + } + + // because instanceof doesn't work with primitive types (e.g. String), taken from https://stackoverflow.com/a/58184883/544947 + public static IsInstanceOf(variable: any, type: any) { + if (TypeHelpers.IsNullOrUndefined(variable)) { + throw new Error( + "Invalid 'variable' parameter passed in: null or undefined" + ); + } + if (TypeHelpers.IsNullOrUndefined(type)) { + throw new Error( + "Invalid 'type' parameter passed in: null or undefined" + ); + } + + let res: boolean = false; + if (typeof type == "string") { + res = typeof variable == type.toLowerCase(); + } else { + res = variable.constructor == type; + } + return res; + } +} diff --git a/commitlint/helpers.ts b/commitlint/helpers.ts index dc4ee720..3b88bf7b 100644 --- a/commitlint/helpers.ts +++ b/commitlint/helpers.ts @@ -1,3 +1,5 @@ +import { None, Some, Option, OptionStatic, TypeHelpers } from "./fpHelpers.js"; + export abstract class Helpers { public static errMessageSuffix = "\nFor reference, these are the guidelines that include our commit message conventions: https://github.com/nblockchain/conventions/blob/master/docs/WorkflowGuidelines.md"; @@ -16,12 +18,25 @@ export abstract class Helpers { } // to convert from 'any' type - public static convertAnyToString( - potentialString: any, - paramName: string - ): string | null { - // this null/undefined check is required, otherwise, String(null) might give us the stupid string "null" - return potentialString ? String(potentialString) : null; + public static convertAnyToString(potentialString: any): Option { + if (TypeHelpers.IsNullOrUndefined(potentialString)) { + return OptionStatic.None; + } + // this type check is required, otherwise, String(null) would give us the stupid string "null" + if (TypeHelpers.IsInstanceOf(potentialString, String)) { + return new Some(String(potentialString)); + } + return OptionStatic.None; + } + + public static assertNotNone( + text: Option, + errorMessage: string + ): string { + if (text instanceof None) { + throw new Error(errorMessage); + } + return text.value; } public static assertNotNull( @@ -75,7 +90,7 @@ export abstract class Helpers { public static findUrls(text: string) { var urlRegex = /(https?:\/\/[^\s]+)/g; - return text.match(urlRegex); + return OptionStatic.OfObj(text.match(urlRegex)); } public static isCommitUrl(url: string) { diff --git a/commitlint/plugins.ts b/commitlint/plugins.ts index 4159dde9..da962c59 100644 --- a/commitlint/plugins.ts +++ b/commitlint/plugins.ts @@ -1,3 +1,4 @@ +import { Option, Some, None, OptionStatic } from "./fpHelpers.js"; import { abbr } from "./abbreviations.js"; import { Helpers } from "./helpers.js"; @@ -90,13 +91,18 @@ export abstract class Plugins { let urls = Helpers.findUrls(rawStr); - let gitRepo = process.env["GITHUB_REPOSITORY"]; - if (gitRepo !== undefined && urls !== null) { - for (let url of urls.entries()) { - let urlStr = url[1].toString(); - if (Helpers.isCommitUrl(urlStr) && urlStr.includes(gitRepo)) { - offence = true; - break; + let gitRepo = OptionStatic.OfObj(process.env["GITHUB_REPOSITORY"]); + if (!(gitRepo instanceof None)) { + if (!(urls instanceof None)) { + for (let url of urls.value.entries()) { + let urlStr = url[1].toString(); + if ( + Helpers.isCommitUrl(urlStr) && + urlStr.includes(gitRepo.value) + ) { + offence = true; + break; + } } } } @@ -154,10 +160,11 @@ export abstract class Plugins { return [!offence, message + Helpers.errMessageSuffix]; } - public static footerNotesMisplacement(bodyStr: string | null) { + public static footerNotesMisplacement(body: Option) { let offence = false; - if (bodyStr !== null) { + if (!(body instanceof None)) { + let bodyStr = body.value; bodyStr = Helpers.removeAllCodeBlocks(bodyStr).trim(); let seenBody = false; let seenFooter = false; @@ -376,12 +383,13 @@ export abstract class Plugins { } public static bodySoftMaxLineLength( - bodyStr: string | null, + body: Option, bodyMaxLineLength: number ) { let offence = false; - if (bodyStr !== null) { + if (!(body instanceof None)) { + let bodyStr = body.value; bodyStr = bodyStr.trim(); bodyStr = Helpers.removeAllCodeBlocks(bodyStr).trim(); @@ -433,13 +441,14 @@ export abstract class Plugins { } public static bodyParagraphLineMinLength( - bodyStr: string | null, + body: Option, paragraphLineMinLength: number, paragraphLineMaxLength: number ) { - let offence: string | null = null; + let offence: Option = OptionStatic.None; - if (bodyStr !== null) { + if (!(body instanceof None)) { + let bodyStr = body.value; bodyStr = Helpers.removeAllCodeBlocks(bodyStr).trim(); let paragraphs = Helpers.splitByEOLs(bodyStr, 2); @@ -502,7 +511,7 @@ export abstract class Plugins { !isLastLineBeforeNextBullet && !isLastCharAColonBreak ) { - offence = line; + offence = new Some(line); break; } } @@ -510,11 +519,15 @@ export abstract class Plugins { } } - let isValid = offence === null; - + if (offence instanceof None) { + return [ + true, + "bodyParagraphLineMinLength's bug, this text should not be shown if offence was false", + ]; + } return [ - isValid, - `Please do not subceed ${paragraphLineMinLength} characters in the lines of the commit message's body paragraphs. Offending line has this text: "${offence}"; we recommend this script (for editing the last commit message): \n` + + false, + `Please do not subceed ${paragraphLineMinLength} characters in the lines of the commit message's body paragraphs. Offending line has this text: "${offence.value}"; we recommend this script (for editing the last commit message): \n` + "https://github.com/nblockchain/conventions/blob/master/scripts/wrapLatestCommitMsg.fsx" + Helpers.errMessageSuffix, ]; diff --git a/commitlint/tests/testHelpers.ts b/commitlint/tests/testHelpers.ts index 872b48ae..f529ede9 100644 --- a/commitlint/tests/testHelpers.ts +++ b/commitlint/tests/testHelpers.ts @@ -1,6 +1,138 @@ +import { test, expect } from "vitest"; +import { None, Some, Option, OptionStatic, TypeHelpers } from "../fpHelpers.js"; const { spawnSync } = require("child_process"); const os = require("os"); +function typeGuard(option: Option) { + if (option instanceof None) { + return "NAH"; + } else { + let val = option.value; + return (val * val).toString(); + } +} + +function ofObj1(option: number | null): Option { + let foo = OptionStatic.OfObj(option); + return foo; +} + +function ofObj2(option: number | undefined): Option { + let foo = OptionStatic.OfObj(option); + return foo; +} + +test("testing Options", () => { + let foo: Option = new None(); + let bar: Option = new Some(2); + expect(typeGuard(foo)).toBe("NAH"); + expect(typeGuard(bar)).toBe("4"); +}); + +test("testing Is methods", () => { + let foo: Option = OptionStatic.None; + let bar: Option = new Some(2); + expect(foo.IsNone()).toBe(true); + expect(bar.IsNone()).toBe(false); + expect(foo.IsSome()).toBe(false); + expect(bar.IsSome()).toBe(true); +}); + +test("testing OfObj", () => { + let two: number | null = 2; + expect(typeGuard(ofObj1(two))).toBe("4"); + two = null; + expect(typeGuard(ofObj1(two))).toBe("NAH"); + + let four: number | undefined = 4; + expect(typeGuard(ofObj2(four))).toBe("16"); + four = undefined; + expect(typeGuard(ofObj2(four))).toBe("NAH"); +}); + +class Foo { + public JustToMakeFooNonEmpty() { + return null; + } +} +class Bar { + public JustToMakeBarNonEmpty() { + return null; + } +} + +test("testing TypeHelpers.IsInstanceOf", () => { + let str1 = "foo"; + expect(TypeHelpers.IsInstanceOf(str1, String)).toBe(true); + let str2 = String("foo"); + expect(TypeHelpers.IsInstanceOf(str2, String)).toBe(true); + + //commented this one because prettier complains about it, but it works: + //let str3 = 'foo'; + //expect(TypeHelpers.IsInstanceOf(str3, String)).toBe(true); + + let nonStr = 3; + expect(TypeHelpers.IsInstanceOf(nonStr, String)).toBe(false); + + let int1 = 2; + expect(TypeHelpers.IsInstanceOf(int1, Number)).toBe(true); + let int2 = Number(2); + expect(TypeHelpers.IsInstanceOf(int2, Number)).toBe(true); + let nonInt = "2"; + expect(TypeHelpers.IsInstanceOf(nonInt, Number)).toBe(false); + + let foo = new Foo(); + let bar = new Bar(); + expect(TypeHelpers.IsInstanceOf(foo, Foo)).toBe(true); + expect(TypeHelpers.IsInstanceOf(bar, Bar)).toBe(true); + expect(TypeHelpers.IsInstanceOf(foo, Bar)).toBe(false); + expect(TypeHelpers.IsInstanceOf(bar, Foo)).toBe(false); +}); + +test("testing TypeHelpers.IsInstanceOf exceptions", () => { + let strNull = null; + expect(() => TypeHelpers.IsInstanceOf(strNull, String)).toThrowError( + "Invalid" + ); + expect(() => TypeHelpers.IsInstanceOf(strNull, String)).toThrowError( + "parameter" + ); + expect(() => TypeHelpers.IsInstanceOf(strNull, String)).toThrowError( + "null" + ); + let strUndefined = undefined; + expect(() => TypeHelpers.IsInstanceOf(strUndefined, String)).toThrowError( + "Invalid" + ); + expect(() => TypeHelpers.IsInstanceOf(strUndefined, String)).toThrowError( + "parameter" + ); + expect(() => TypeHelpers.IsInstanceOf(strUndefined, String)).toThrowError( + "undefined" + ); + + let typeNull = null; + expect(() => TypeHelpers.IsInstanceOf("foo", typeNull)).toThrowError( + "Invalid" + ); + expect(() => TypeHelpers.IsInstanceOf("foo", typeNull)).toThrowError( + "parameter" + ); + expect(() => TypeHelpers.IsInstanceOf("foo", typeNull)).toThrowError( + "null" + ); + let typeUndefined = undefined; + expect(() => TypeHelpers.IsInstanceOf("foo", typeUndefined)).toThrowError( + "Invalid" + ); + expect(() => TypeHelpers.IsInstanceOf("foo", typeUndefined)).toThrowError( + "parameter" + ); + expect(() => TypeHelpers.IsInstanceOf("foo", typeUndefined)).toThrowError( + "undefined" + ); +}); + export function runCommitLintOnMsg(inputMsg: string) { // FIXME: should we .lowerCase().startsWith("win") in case it starts // returning Win64 in the future? thing is, our CI doesn't like this diff --git a/docs/WorkflowGuidelines.md b/docs/WorkflowGuidelines.md index 3fbb60d7..a5f2f35f 100644 --- a/docs/WorkflowGuidelines.md +++ b/docs/WorkflowGuidelines.md @@ -163,9 +163,8 @@ make use of the type system whenever you can, for example: * Do not use edge-values to denote absence of value. Example: use null (`Nullable`) instead of `DateTime.MinValue`. - * Use Option types instead of Nullable ones if your language provides it (e.g. if you're using F# instead of C#). * Do not use `undefined` which is a pitfall from JavaScript (the fact that it has two kinds of null values is a defect in - its design). As we're using TypeScript we should be able to avoid the uglyness of JavaScript. + its design). As we're using TypeScript we should be able to avoid the ugliness of JavaScript. Example (with bad practice): ```typescript @@ -173,6 +172,26 @@ { return 0; } + return 1; + ``` + + Improved code: + ```typescript + if (TypeHelpers.IsNullOrUndefined(foo)) + { + return 0; + } + return 1; + ``` + + * Use Option types instead of Nullable ones if your language provides it (e.g. if you're using F# instead of C#). + + Example (with bad practice): + ```typescript + if (TypeHelpers.IsNullOrUndefined(foo)) + { + return 0; + } else { return 1; @@ -181,7 +200,11 @@ Improved code: ```typescript - return foo ? 1 : 0; + let bar = OptionStatic.OfObj(option); + if (bar instanceof None) { + return 0; + } + return 1; ``` * Abusing obscure operators or the excessive multi-facetedness of basic ones: