From 6e4cc33565dcb17db620e8eac9f38cc552c731ef Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Thu, 5 Sep 2024 16:20:42 +0800 Subject: [PATCH 01/10] commitlint: refactor, remove unneeded param --- commitlint.config.ts | 36 ++++++++++++++++++------------------ commitlint/helpers.ts | 5 +---- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/commitlint.config.ts b/commitlint.config.ts index eec5eb87..2f208321 100644 --- a/commitlint.config.ts +++ b/commitlint.config.ts @@ -65,7 +65,7 @@ export default { rules: { "body-prose": ({ raw }: { raw: any }) => { let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw, "raw"), + Helpers.convertAnyToString(raw), notNullStringErrorMessage("raw") ); @@ -74,7 +74,7 @@ export default { "commit-hash-alone": ({ raw }: { raw: any }) => { let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw, "raw"), + Helpers.convertAnyToString(raw), notNullStringErrorMessage("raw") ); @@ -83,7 +83,7 @@ export default { "empty-wip": ({ header }: { header: any }) => { let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), + Helpers.convertAnyToString(header), notNullStringErrorMessage("header") ); @@ -96,7 +96,7 @@ export default { maxLineLength: number ) => { let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), + Helpers.convertAnyToString(header), notNullStringErrorMessage("header") ); @@ -107,13 +107,13 @@ export default { }, "footer-notes-misplacement": ({ body }: { body: any }) => { - let bodyStr = Helpers.convertAnyToString(body, "body"); + let bodyStr = Helpers.convertAnyToString(body); return Plugins.footerNotesMisplacement(bodyStr); }, "footer-refs-validity": ({ raw }: { raw: any }) => { let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw, "raw"), + Helpers.convertAnyToString(raw), notNullStringErrorMessage("raw") ); @@ -126,7 +126,7 @@ export default { header: any; }) => { let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), + Helpers.convertAnyToString(header), notNullStringErrorMessage("header") ); @@ -135,7 +135,7 @@ export default { "proper-issue-refs": ({ raw }: { raw: any }) => { let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw, "raw"), + Helpers.convertAnyToString(raw), notNullStringErrorMessage("raw") ); @@ -144,7 +144,7 @@ export default { "title-uppercase": ({ header }: { header: any }) => { let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), + Helpers.convertAnyToString(header), notNullStringErrorMessage("header") ); @@ -153,7 +153,7 @@ export default { "too-many-spaces": ({ raw }: { raw: any }) => { let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw, "raw"), + Helpers.convertAnyToString(raw), notNullStringErrorMessage("raw") ); @@ -162,7 +162,7 @@ export default { "type-space-after-colon": ({ header }: { header: any }) => { let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), + Helpers.convertAnyToString(header), notNullStringErrorMessage("header") ); @@ -171,7 +171,7 @@ export default { "type-with-square-brackets": ({ header }: { header: any }) => { let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), + Helpers.convertAnyToString(header), notNullStringErrorMessage("header") ); @@ -181,7 +181,7 @@ 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"), + Helpers.convertAnyToString(header), notNullStringErrorMessage("header") ); return Plugins.subjectLowercase(headerStr); @@ -189,7 +189,7 @@ export default { "type-space-after-comma": ({ header }: { header: any }) => { let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), + Helpers.convertAnyToString(header), notNullStringErrorMessage("header") ); @@ -201,7 +201,7 @@ export default { _: any, maxLineLength: number ) => { - let bodyStr = Helpers.convertAnyToString(body, "body"); + let bodyStr = Helpers.convertAnyToString(body); return Plugins.bodySoftMaxLineLength( bodyStr, maxLineLength @@ -209,7 +209,7 @@ export default { }, "body-paragraph-line-min-length": ({ body }: { body: any }) => { - let bodyStr = Helpers.convertAnyToString(body, "body"); + let bodyStr = Helpers.convertAnyToString(body); return Plugins.bodyParagraphLineMinLength( bodyStr, headerMaxLineLength, @@ -219,7 +219,7 @@ export default { "trailing-whitespace": ({ raw }: { raw: any }) => { let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw, "raw"), + Helpers.convertAnyToString(raw), notNullStringErrorMessage("raw") ); @@ -228,7 +228,7 @@ export default { "type-space-before-paren": ({ header }: { header: any }) => { let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(header, "header"), + Helpers.convertAnyToString(header), notNullStringErrorMessage("header") ); diff --git a/commitlint/helpers.ts b/commitlint/helpers.ts index dc4ee720..8c788e85 100644 --- a/commitlint/helpers.ts +++ b/commitlint/helpers.ts @@ -16,10 +16,7 @@ export abstract class Helpers { } // to convert from 'any' type - public static convertAnyToString( - potentialString: any, - paramName: string - ): string | null { + public static convertAnyToString(potentialString: any): string | null { // this null/undefined check is required, otherwise, String(null) might give us the stupid string "null" return potentialString ? String(potentialString) : null; } From 1c9b0c48c503e36741e08f92bb03c48d788da306 Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Thu, 5 Sep 2024 15:54:59 +0800 Subject: [PATCH 02/10] commitlint: introduce an Option type And make use of it in one of the plugins already. See my SO post about this: https://stackoverflow.com/a/78937127/544947 Co-authored-by: webwarrior --- commitlint/fpHelpers.ts | 9 +++++++++ commitlint/plugins.ts | 17 +++++++++++------ commitlint/tests/testHelpers.ts | 18 ++++++++++++++++++ 3 files changed, 38 insertions(+), 6 deletions(-) create mode 100644 commitlint/fpHelpers.ts diff --git a/commitlint/fpHelpers.ts b/commitlint/fpHelpers.ts new file mode 100644 index 00000000..d01c94f1 --- /dev/null +++ b/commitlint/fpHelpers.ts @@ -0,0 +1,9 @@ +export class None {} +export class Some { + value: T; + + constructor(val: T) { + this.value = val; + } +} +export type Option = None | Some; diff --git a/commitlint/plugins.ts b/commitlint/plugins.ts index 4159dde9..5eafec5c 100644 --- a/commitlint/plugins.ts +++ b/commitlint/plugins.ts @@ -1,3 +1,4 @@ +import { Option, Some, None } from "./fpHelpers.js"; import { abbr } from "./abbreviations.js"; import { Helpers } from "./helpers.js"; @@ -437,7 +438,7 @@ export abstract class Plugins { paragraphLineMinLength: number, paragraphLineMaxLength: number ) { - let offence: string | null = null; + let offence: Option = new None(); if (bodyStr !== null) { bodyStr = Helpers.removeAllCodeBlocks(bodyStr).trim(); @@ -502,7 +503,7 @@ export abstract class Plugins { !isLastLineBeforeNextBullet && !isLastCharAColonBreak ) { - offence = line; + offence = new Some(line); break; } } @@ -510,11 +511,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..5d4b9392 100644 --- a/commitlint/tests/testHelpers.ts +++ b/commitlint/tests/testHelpers.ts @@ -1,6 +1,24 @@ +import { test, expect } from "vitest"; +import { None, Some, Option } 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(); + } +} + +test("testing Options", () => { + let foo: Option = new None(); + let bar: Option = new Some(2); + expect(typeGuard(foo)).toBe("NAH"); + expect(typeGuard(bar)).toBe("4"); +}); + 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 From e5a23bd7db1dc43db8ad2c901def120c1d04c102 Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Thu, 5 Sep 2024 21:17:38 +0800 Subject: [PATCH 03/10] commitlint: do not use empty class for None Empty classes result in strange issues that turn the TS compiler into not very helpful. E.g.: https://stackoverflow.com/questions/78953267/why-is-typescript-not-throwing-a-compile-time-error-when-constructor-is-not-used NOTE: we still mark the new methods as deprecated to give a hint that `instanceof` usage is better. These tooltips will be properly shown by IDEs and linters according to: https://stackoverflow.com/a/62991642/544947 --- commitlint/fpHelpers.ts | 30 ++++++++++++++++++++++++++++-- commitlint/tests/testHelpers.ts | 9 +++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/commitlint/fpHelpers.ts b/commitlint/fpHelpers.ts index d01c94f1..37e022c0 100644 --- a/commitlint/fpHelpers.ts +++ b/commitlint/fpHelpers.ts @@ -1,9 +1,35 @@ -export class None {} +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; + } +} export class Some { value: T; constructor(val: T) { this.value = val; } + + public IsNone(): boolean { + return false; + } + public IsSome(): boolean { + return true; + } } -export type Option = None | Some; + +export type Option = (None | Some) & IOption; diff --git a/commitlint/tests/testHelpers.ts b/commitlint/tests/testHelpers.ts index 5d4b9392..0cd6497e 100644 --- a/commitlint/tests/testHelpers.ts +++ b/commitlint/tests/testHelpers.ts @@ -19,6 +19,15 @@ test("testing Options", () => { expect(typeGuard(bar)).toBe("4"); }); +test("testing Is methods", () => { + let foo: Option = new 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); +}); + 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 From 5056f059307530d5c04864ed3a0f03d84e80e745 Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Fri, 6 Sep 2024 12:10:58 +0800 Subject: [PATCH 04/10] commitlint: add OptionStatic.OfObj and .None --- commitlint/fpHelpers.ts | 20 ++++++++++++++++++-- commitlint/tests/testHelpers.ts | 26 ++++++++++++++++++++++++-- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/commitlint/fpHelpers.ts b/commitlint/fpHelpers.ts index 37e022c0..33f950e7 100644 --- a/commitlint/fpHelpers.ts +++ b/commitlint/fpHelpers.ts @@ -16,11 +16,16 @@ export class None { public IsSome(): boolean { return false; } + + /** + * @deprecated it is better to use `OptionStatic.None` + **/ + constructor() {} } export class Some { value: T; - constructor(val: T) { + constructor(val: NonNullable) { this.value = val; } @@ -32,4 +37,15 @@ export class Some { } } -export type Option = (None | Some) & IOption; +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); + } + } +} diff --git a/commitlint/tests/testHelpers.ts b/commitlint/tests/testHelpers.ts index 0cd6497e..d86f9eb8 100644 --- a/commitlint/tests/testHelpers.ts +++ b/commitlint/tests/testHelpers.ts @@ -1,5 +1,5 @@ import { test, expect } from "vitest"; -import { None, Some, Option } from "../fpHelpers.js"; +import { None, Some, Option, OptionStatic } from "../fpHelpers.js"; const { spawnSync } = require("child_process"); const os = require("os"); @@ -12,6 +12,16 @@ function typeGuard(option: Option) { } } +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); @@ -20,7 +30,7 @@ test("testing Options", () => { }); test("testing Is methods", () => { - let foo: Option = new None(); + let foo: Option = OptionStatic.None; let bar: Option = new Some(2); expect(foo.IsNone()).toBe(true); expect(bar.IsNone()).toBe(false); @@ -28,6 +38,18 @@ test("testing Is methods", () => { 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"); +}); + 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 From 539937f8f3214d3cd02bcb7b0ce8a49e48f8f7ae Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Fri, 6 Sep 2024 12:59:01 +0800 Subject: [PATCH 05/10] commitlint: introduce TypeHelpers.IsInstanceOf The keyword `instanceof` is a footgun in JS/TS because it only works for classes, but not primitive types. This was taken from: https://stackoverflow.com/a/58184883/544947 --- commitlint/fpHelpers.ts | 13 +++++++++++ commitlint/tests/testHelpers.ts | 41 ++++++++++++++++++++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/commitlint/fpHelpers.ts b/commitlint/fpHelpers.ts index 33f950e7..5af094d2 100644 --- a/commitlint/fpHelpers.ts +++ b/commitlint/fpHelpers.ts @@ -49,3 +49,16 @@ export class OptionStatic { } } } + +export class TypeHelpers { + // 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) { + let res: boolean = false; + if (typeof type == "string") { + res = typeof variable == type.toLowerCase(); + } else { + res = variable.constructor == type; + } + return res; + } +} diff --git a/commitlint/tests/testHelpers.ts b/commitlint/tests/testHelpers.ts index d86f9eb8..c2a05026 100644 --- a/commitlint/tests/testHelpers.ts +++ b/commitlint/tests/testHelpers.ts @@ -1,5 +1,5 @@ import { test, expect } from "vitest"; -import { None, Some, Option, OptionStatic } from "../fpHelpers.js"; +import { None, Some, Option, OptionStatic, TypeHelpers } from "../fpHelpers.js"; const { spawnSync } = require("child_process"); const os = require("os"); @@ -50,6 +50,45 @@ test("testing OfObj", () => { 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); +}); + 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 From 0d40db613790600aaa3a2e900a9d32075059abd9 Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Tue, 10 Sep 2024 00:46:28 +0800 Subject: [PATCH 06/10] fpHelpers(IsInstanceOf): reject "nothing" values --- commitlint/fpHelpers.ts | 11 +++++++++ commitlint/tests/testHelpers.ts | 44 +++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/commitlint/fpHelpers.ts b/commitlint/fpHelpers.ts index 5af094d2..302e85e0 100644 --- a/commitlint/fpHelpers.ts +++ b/commitlint/fpHelpers.ts @@ -53,6 +53,17 @@ export class OptionStatic { export class TypeHelpers { // 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 (variable === null || variable === undefined) { + throw new Error( + "Invalid 'variable' parameter passed in: null or undefined" + ); + } + if (type === null || type === undefined) { + throw new Error( + "Invalid 'type' parameter passed in: null or undefined" + ); + } + let res: boolean = false; if (typeof type == "string") { res = typeof variable == type.toLowerCase(); diff --git a/commitlint/tests/testHelpers.ts b/commitlint/tests/testHelpers.ts index c2a05026..f529ede9 100644 --- a/commitlint/tests/testHelpers.ts +++ b/commitlint/tests/testHelpers.ts @@ -89,6 +89,50 @@ test("testing TypeHelpers.IsInstanceOf", () => { 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 From 2960c7c2d96165b6e58c2cc5679250189485a4f6 Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Tue, 10 Sep 2024 12:30:56 +0800 Subject: [PATCH 07/10] fpHelpers: extract TypeHelpers.IsNullOrUndefined() --- commitlint/fpHelpers.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/commitlint/fpHelpers.ts b/commitlint/fpHelpers.ts index 302e85e0..3b7de9c5 100644 --- a/commitlint/fpHelpers.ts +++ b/commitlint/fpHelpers.ts @@ -51,14 +51,18 @@ export class OptionStatic { } 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 (variable === null || variable === undefined) { + if (TypeHelpers.IsNullOrUndefined(variable)) { throw new Error( "Invalid 'variable' parameter passed in: null or undefined" ); } - if (type === null || type === undefined) { + if (TypeHelpers.IsNullOrUndefined(type)) { throw new Error( "Invalid 'type' parameter passed in: null or undefined" ); From 2100e2cb54d3232a9de75252317334b274f8c2cb Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Tue, 10 Sep 2024 15:10:49 +0800 Subject: [PATCH 08/10] docs: fix typo --- docs/WorkflowGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/WorkflowGuidelines.md b/docs/WorkflowGuidelines.md index 3fbb60d7..42b67893 100644 --- a/docs/WorkflowGuidelines.md +++ b/docs/WorkflowGuidelines.md @@ -165,7 +165,7 @@ * 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 From 34c5a378a86c8e5b912deaa5eb5e975dce8dcf86 Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Fri, 6 Sep 2024 13:47:55 +0800 Subject: [PATCH 09/10] commitlint: start using FP patterns --- commitlint.config.ts | 110 ++++++++++++++++++++---------------------- commitlint/helpers.ts | 26 ++++++++-- commitlint/plugins.ts | 38 +++++++++------ 3 files changed, 97 insertions(+), 77 deletions(-) diff --git a/commitlint.config.ts b/commitlint.config.ts index 2f208321..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), - notNullStringErrorMessage("raw") - ); + let rawStr = extractStringFromCommitlintParam("raw", raw); return Plugins.bodyProse(rawStr); }, "commit-hash-alone": ({ raw }: { raw: any }) => { - let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw), - notNullStringErrorMessage("raw") - ); + let rawStr = extractStringFromCommitlintParam("raw", raw); return Plugins.commitHashAlone(rawStr); }, "empty-wip": ({ header }: { header: any }) => { - let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(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), - 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); - 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), - 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), - notNullStringErrorMessage("header") + let headerStr = extractStringFromCommitlintParam( + "header", + header ); return Plugins.preferSlashOverBackslash(headerStr); }, "proper-issue-refs": ({ raw }: { raw: any }) => { - let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(raw), - notNullStringErrorMessage("raw") - ); + let rawStr = extractStringFromCommitlintParam("raw", raw); return Plugins.properIssueRefs(rawStr); }, "title-uppercase": ({ header }: { header: any }) => { - let headerStr = Helpers.assertNotNull( - Helpers.convertAnyToString(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), - 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), - 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), - 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), - notNullStringErrorMessage("header") + let headerStr = extractStringFromCommitlintParam( + "header", + header ); + return Plugins.subjectLowercase(headerStr); }, "type-space-after-comma": ({ header }: { header: any }) => { - let headerStr = Helpers.assertNotNull( + let headerStr = Helpers.assertNotNone( Helpers.convertAnyToString(header), - notNullStringErrorMessage("header") + notStringErrorMessage("header") ); return Plugins.typeSpaceAfterComma(headerStr); @@ -201,35 +198,32 @@ export default { _: any, maxLineLength: number ) => { - let bodyStr = Helpers.convertAnyToString(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); + let maybeBody = Helpers.convertAnyToString(body); return Plugins.bodyParagraphLineMinLength( - bodyStr, + maybeBody, headerMaxLineLength, bodyMaxLineLength ); }, "trailing-whitespace": ({ raw }: { raw: any }) => { - let rawStr = Helpers.assertNotNull( - Helpers.convertAnyToString(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), - notNullStringErrorMessage("header") + let headerStr = extractStringFromCommitlintParam( + "header", + header ); return Plugins.typeSpaceBeforeParen(headerStr); diff --git a/commitlint/helpers.ts b/commitlint/helpers.ts index 8c788e85..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,9 +18,25 @@ export abstract class Helpers { } // to convert from 'any' type - public static convertAnyToString(potentialString: any): 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( @@ -72,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 5eafec5c..da962c59 100644 --- a/commitlint/plugins.ts +++ b/commitlint/plugins.ts @@ -1,4 +1,4 @@ -import { Option, Some, None } from "./fpHelpers.js"; +import { Option, Some, None, OptionStatic } from "./fpHelpers.js"; import { abbr } from "./abbreviations.js"; import { Helpers } from "./helpers.js"; @@ -91,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; + } } } } @@ -155,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; @@ -377,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(); @@ -434,13 +441,14 @@ export abstract class Plugins { } public static bodyParagraphLineMinLength( - bodyStr: string | null, + body: Option, paragraphLineMinLength: number, paragraphLineMaxLength: number ) { - let offence: Option = new None(); + 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); From 6807c3d92ae5d8d821c66bd005617196a392475f Mon Sep 17 00:00:00 2001 From: "Andres G. Aragoneses" Date: Tue, 10 Sep 2024 15:12:32 +0800 Subject: [PATCH 10/10] docs/WorkflowGuidelines.md: improve FP rules --- docs/WorkflowGuidelines.md | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/docs/WorkflowGuidelines.md b/docs/WorkflowGuidelines.md index 42b67893..a5f2f35f 100644 --- a/docs/WorkflowGuidelines.md +++ b/docs/WorkflowGuidelines.md @@ -163,7 +163,6 @@ 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 ugliness of JavaScript. @@ -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: