Skip to content

A new rule that makes sure the user have explained the reason of reverting #190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 55 additions & 25 deletions commitlint.config.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Helpers } from "./commitlint/helpers.js";
import { None, OptionStatic } from "./commitlint/fpHelpers.js";
import { Helpers, When } from "./commitlint/helpers.js";
import { Plugins } from "./commitlint/plugins.js";
import { RuleConfigSeverity } from "@commitlint/types";

Expand All @@ -24,48 +25,55 @@ function extractStringFromCommitlintParam(
export default {
parserPreset: "conventional-changelog-conventionalcommits",
rules: {
"body-leading-blank": [RuleConfigSeverity.Error, "always"],
"body-leading-blank": [RuleConfigSeverity.Error, When.Always],
"body-soft-max-line-length": [
RuleConfigSeverity.Error,
"always",
When.Always,
bodyMaxLineLength,
],
"body-paragraph-line-min-length": [RuleConfigSeverity.Error, "always"],
"empty-wip": [RuleConfigSeverity.Error, "always"],
"footer-leading-blank": [RuleConfigSeverity.Warning, "always"],
"body-paragraph-line-min-length": [
RuleConfigSeverity.Error,
When.Always,
],
"empty-wip": [RuleConfigSeverity.Error, When.Always],
"footer-leading-blank": [RuleConfigSeverity.Warning, When.Always],
"footer-max-line-length": [
RuleConfigSeverity.Error,
"always",
When.Always,
footerMaxLineLength,
],
"footer-notes-misplacement": [RuleConfigSeverity.Error, "always"],
"footer-refs-validity": [RuleConfigSeverity.Error, "always"],
"footer-notes-misplacement": [RuleConfigSeverity.Error, When.Always],
"footer-refs-validity": [RuleConfigSeverity.Error, When.Always],
"header-max-length-with-suggestions": [
RuleConfigSeverity.Error,
"always",
When.Always,
headerMaxLineLength,
],
"subject-full-stop": [RuleConfigSeverity.Error, "never", "."],
"type-space-after-colon": [RuleConfigSeverity.Error, "always"],
"subject-lowercase": [RuleConfigSeverity.Error, "always"],
"body-prose": [RuleConfigSeverity.Error, "always"],
"type-space-after-comma": [RuleConfigSeverity.Error, "always"],
"trailing-whitespace": [RuleConfigSeverity.Error, "always"],
"prefer-slash-over-backslash": [RuleConfigSeverity.Error, "always"],
"type-space-before-paren": [RuleConfigSeverity.Error, "always"],
"type-with-square-brackets": [RuleConfigSeverity.Error, "always"],
"proper-issue-refs": [RuleConfigSeverity.Error, "always"],
"too-many-spaces": [RuleConfigSeverity.Error, "always"],
"commit-hash-alone": [RuleConfigSeverity.Error, "always"],
"title-uppercase": [RuleConfigSeverity.Error, "always"],
"subject-full-stop": [RuleConfigSeverity.Error, When.Never, "."],
"type-space-after-colon": [RuleConfigSeverity.Error, When.Always],
"subject-lowercase": [RuleConfigSeverity.Error, When.Always],
"body-prose": [RuleConfigSeverity.Error, When.Always],
"type-space-after-comma": [RuleConfigSeverity.Error, When.Always],
"trailing-whitespace": [RuleConfigSeverity.Error, When.Always],
"prefer-slash-over-backslash": [RuleConfigSeverity.Error, When.Always],
"type-space-before-paren": [RuleConfigSeverity.Error, When.Always],
"type-with-square-brackets": [RuleConfigSeverity.Error, When.Always],
"proper-issue-refs": [RuleConfigSeverity.Error, When.Always],
"too-many-spaces": [RuleConfigSeverity.Error, When.Always],
"commit-hash-alone": [RuleConfigSeverity.Error, When.Always],
"title-uppercase": [RuleConfigSeverity.Error, When.Always],

// disabled because most of the time it doesn't work, due to https://github.com/conventional-changelog/commitlint/issues/3404
// and anyway we were using this rule only as a warning, not an error (because a scope is not required, e.g. when too broad)
"type-empty": [RuleConfigSeverity.Disabled, "never"],
"type-empty": [RuleConfigSeverity.Disabled, When.Never],
"default-revert-message": [RuleConfigSeverity.Error, When.Never],
},

// Commitlint automatically ignores some kinds of commits like Revert commit messages.
// We need to set this value to false to apply our rules on these messages.
defaultIgnores: false,
plugins: [
// TODO (ideas for more rules):
// * Detect reverts which have not been elaborated.
// * Reject some stupid obvious words: change, update, modify (if first word after colon, error; otherwise warning).
// * Think of how to reject this shitty commit message: https://github.com/nblockchain/NOnion/pull/34/commits/9ffcb373a1147ed1c729e8aca4ffd30467255594
// * Workflow: detect if wip commit in a branch not named "wip/*" or whose name contains "squashed".
Expand Down Expand Up @@ -141,6 +149,28 @@ export default {
return Plugins.properIssueRefs(rawStr);
},

"default-revert-message": (
{
header,
body,
}: {
header: any;
body: any;
},
when: string
) => {
let bodyStr = Helpers.convertAnyToString(body);
let headerStr = Helpers.assertNotNone(
Helpers.convertAnyToString(header),
notStringErrorMessage("header")
);
return Plugins.defaultRevertMessage(
headerStr,
bodyStr instanceof None ? null : bodyStr.value,
Helpers.assertWhen(when)
);
},

"title-uppercase": ({ header }: { header: any }) => {
let headerStr = extractStringFromCommitlintParam(
"header",
Expand Down
14 changes: 14 additions & 0 deletions commitlint/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import { None, Some, Option, OptionStatic, TypeHelpers } from "./fpHelpers.js";

export enum When {
Never = "never",
Always = "always",
}

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";
Expand Down Expand Up @@ -49,6 +54,15 @@ export abstract class Helpers {
return text as string;
}

public static assertWhen(when: string) {
if (when !== When.Never && when !== When.Always) {
throw new Error(
'Variable "when" should be either "never" or "always"'
);
}
return when as When;
}

public static assertCharacter(letter: string) {
if (letter.length !== 1) {
throw Error("This function expects a character as input");
Expand Down
40 changes: 39 additions & 1 deletion commitlint/plugins.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Option, Some, None, OptionStatic } from "./fpHelpers.js";
import { abbr } from "./abbreviations.js";
import { Helpers } from "./helpers.js";
import { Helpers, When } from "./helpers.js";

export abstract class Plugins {
public static bodyProse(rawStr: string) {
Expand Down Expand Up @@ -286,6 +286,44 @@ export abstract class Plugins {
];
}

public static defaultRevertMessage(
headerStr: string,
bodyStr: string | null,
when: When
) {
let offence = false;
let isRevertCommitMessage = headerStr.toLowerCase().includes("revert");

const negated = when === "never";

if (isRevertCommitMessage) {
let isDefaultRevertHeader =
headerStr.match(/^[Rr]evert ".+"$/) !== null;

if (isDefaultRevertHeader) {
if (bodyStr !== null) {
let lines = bodyStr.split("\n");
offence =
lines.length == 1 &&
// 40 is the length of git commit hash.
lines[0].match(/^This reverts commit [^ ]{40}\.$/) !==
null;
} else {
offence = true;
}
}

offence = negated ? offence : !offence;
}
return [
!offence,
(negated
? `Please explain why you're reverting.`
: `Please don't change the default revert commit message.`) +
Helpers.errMessageSuffix,
];
}

public static titleUppercase(headerStr: string) {
let firstWord = headerStr.split(" ")[0];
let offence =
Expand Down
68 changes: 68 additions & 0 deletions commitlint/tests/plugins.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,74 @@ test("proper-issue-refs5", () => {
expect(properIssueRefs5.status).not.toBe(0);
});

test("default-revert-message1", () => {
let commitMsgWithoutDefaultRevertMessage =
'Revert "add abbreviations.ts"\n\n' +
"This reverts commit 0272f587c7eece147e8d1756116b0b43e11c34ac.";
let defaultRevertMessage1 = runCommitLintOnMsg(
commitMsgWithoutDefaultRevertMessage
);
expect(defaultRevertMessage1.status).not.toBe(0);
});

test("default-revert-message2", () => {
let commitMsgWithDefaultRevertMessage =
'Revert "add abbreviations.ts"\n\n' +
"This reverts commit 0272f587c7eece147e8d1756116b0b43e11c34ac\n" +
"because/otherwise bla bla.";
let defaultRevertMessage2 = runCommitLintOnMsg(
commitMsgWithDefaultRevertMessage
);
expect(defaultRevertMessage2.status).toBe(0);
});

test("default-revert-message3", () => {
let commitMsgWithoutDefaultRevertMessage = 'Revert "add abbreviations.ts"';
let defaultRevertMessage3 = runCommitLintOnMsg(
commitMsgWithoutDefaultRevertMessage
);
expect(defaultRevertMessage3.status).not.toBe(0);
});

test("default-revert-message4", () => {
let commitMsgWithDefaultRevertMessage = "Revert .NET6 upd as it broke CI";
let defaultRevertMessage4 = runCommitLintOnMsg(
commitMsgWithDefaultRevertMessage
);
expect(defaultRevertMessage4.status).toBe(0);
});

test("default-revert-message5", () => {
let commitMsgWithoutDefaultRevertMessage =
'Revert "add abbreviations.ts"\n\n' +
"This reverts commit 0272f587 because bla bla.\n";

let defaultRevertMessage5 = runCommitLintOnMsg(
commitMsgWithoutDefaultRevertMessage
);
expect(defaultRevertMessage5.status).toBe(0);
});

test("default-revert-message6", () => {
let commitMsgWithDefaultRevertMessage =
'Revert "process overhaul" to fix CI\n\n';

let defaultRevertMessage6 = runCommitLintOnMsg(
commitMsgWithDefaultRevertMessage
);
expect(defaultRevertMessage6.status).toBe(0);
});

test("default-revert-message7", () => {
let commitMsgWithoutDefaultRevertMessage =
"This is a revert commit\n\nBla bla.";

let defaultRevertMessage7 = runCommitLintOnMsg(
commitMsgWithoutDefaultRevertMessage
);
expect(defaultRevertMessage7.status).toBe(0);
});

test("subject-lowercase1", () => {
let commitMsgWithUppercaseAfterColon = "foo: Bar baz";
let subjectLowerCase1 = runCommitLintOnMsg(
Expand Down