Skip to content
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

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

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
79 changes: 54 additions & 25 deletions commitlint.config.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Helpers } from "./commitlint/helpers";
import { Helpers, When } from "./commitlint/helpers";
import { Plugins } from "./commitlint/plugins";
import { RuleConfigSeverity } from "@commitlint/types";

Expand All @@ -13,48 +13,55 @@ function notNullStringErrorMessage(stringType: string): string {
module.exports = {
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 @@ -142,6 +149,28 @@ module.exports = {
return Plugins.properIssueRefs(rawStr);
},

"default-revert-message": (
{
header,
body,
}: {
header: any;
body: any;
},
when: string
) => {
let bodyStr = Helpers.convertAnyToString(body, "body");
let headerStr = Helpers.assertNotNull(
Helpers.convertAnyToString(header, "header"),
notNullStringErrorMessage("header")
);
return Plugins.defaultRevertMessage(
headerStr,
bodyStr,
Helpers.assertWhen(when)
);
},

"title-uppercase": ({ header }: { header: any }) => {
let headerStr = Helpers.assertNotNull(
Helpers.convertAnyToString(header, "header"),
Expand Down
14 changes: 14 additions & 0 deletions commitlint/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
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/WorkflowGuidelines.md";
Expand Down Expand Up @@ -37,6 +42,15 @@ export abstract class Helpers {
return text as string;
}

public static assertWhen(when: string) {
tehraninasab marked this conversation as resolved.
Show resolved Hide resolved
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
68 changes: 68 additions & 0 deletions commitlint/plugins.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,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
40 changes: 39 additions & 1 deletion commitlint/plugins.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { abbr } from "./abbreviations";
import { Helpers } from "./helpers";
import { Helpers, When } from "./helpers";

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@realmarv shouldn't it be "startsWith" instead of "includes"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone changes the commit title isn't it possible that "revert" word comes somewhere else in the title?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in which would mean that the revert message used is not default, so if when=never, then offence should be false in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test for this case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, but why do I still see the use of includes()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this variable is checking if the commit message is a revert commit message not if it's a default revert message


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,
tehraninasab marked this conversation as resolved.
Show resolved Hide resolved
];
}

public static titleUppercase(headerStr: string) {
let firstWord = headerStr.split(" ")[0];
let offence =
Expand Down
Loading