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.

Sign up for GitHub

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

Jump to bottom

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

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

wants to merge 4 commits into from

Conversation

tehraninasab
Copy link
Contributor

Replaced #32

@tehraninasab tehraninasab mentioned this pull request Feb 8, 2023
Closed
knocte
knocte reviewed Feb 8, 2023
View reviewed changes
commitlint/helpers.ts Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 8, 2023
View reviewed changes
commitlint/plugins.ts Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 8, 2023
View reviewed changes
commitlint/plugins.ts Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 8, 2023
View reviewed changes
commitlint/plugins.ts Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 8, 2023
View reviewed changes
commitlint.config.ts Outdated Show resolved Hide resolved
@knocte
Copy link
Member

knocte commented Feb 10, 2023

This PR should also remove one TODO line from commitling.config.ts.

@tehraninasab tehraninasab force-pushed the properRevertMessage-squashed-and-rebased branch from cfe2d56 to ac427b8 Compare February 13, 2023 12:33
knocte
knocte reviewed Feb 13, 2023
View reviewed changes
commitlint/plugins.ts Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 13, 2023
View reviewed changes
commitlint/helpers.ts Outdated Show resolved Hide resolved
@knocte
Copy link
Member

knocte commented Feb 14, 2023

@realmarv let's now rebase this PR 🙏

@knocte knocte assigned knocte and tehraninasab and unassigned knocte Feb 14, 2023
@tehraninasab tehraninasab force-pushed the properRevertMessage-squashed-and-rebased branch from e9a1695 to c9d6354 Compare February 14, 2023 10:22
knocte
knocte reviewed Feb 14, 2023
View reviewed changes
commitlint.config.ts Outdated Show resolved Hide resolved
@tehraninasab tehraninasab force-pushed the properRevertMessage-squashed-and-rebased branch 3 times, most recently from fc8a9b3 to 45e9e17 Compare February 15, 2023 11:43
@tehraninasab
Copy link
Contributor Author

@realmarv let's now rebase this PR pray

Done

knocte
knocte reviewed Feb 15, 2023
View reviewed changes
commitlint/plugins.ts Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 15, 2023
View reviewed changes
commitlint/plugins.ts Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 15, 2023
View reviewed changes
commitlint/plugins.ts Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 15, 2023
View reviewed changes
commitlint/plugins.ts Outdated Show resolved Hide resolved
@tehraninasab tehraninasab force-pushed the properRevertMessage-squashed-and-rebased branch 5 times, most recently from e1c6413 to 5d52748 Compare February 15, 2023 12:41
knocte
knocte reviewed Feb 15, 2023
View reviewed changes
commitlint/plugins.ts Outdated Show resolved Hide resolved
@tehraninasab tehraninasab force-pushed the properRevertMessage-squashed-and-rebased branch from 5d52748 to ddaa2f9 Compare February 15, 2023 12:55
knocte
knocte reviewed Feb 15, 2023
View reviewed changes
commitlint.config.ts Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 15, 2023
View reviewed changes
commitlint/plugins.ts Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 15, 2023
View reviewed changes
commitlint/plugins.ts
when: string
) {
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

@tehraninasab tehraninasab force-pushed the properRevertMessage-squashed-and-rebased branch 4 times, most recently from 5187a83 to 560b21f Compare February 20, 2023 12:48
knocte
knocte reviewed Feb 25, 2023
View reviewed changes
commitlint/helpers.ts Outdated
@@ -37,6 +37,14 @@ export abstract class Helpers {
return text as string;
}

public static assertWhen(when: string) {
if (when !== "never" && when !== "always") {
Copy link
Member

Choose a reason for hiding this comment

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

@realmarv I've realised we should create an enum called "When" that has both When.Never and When.Always (but instead of mapping to int, they map to string); hopefully typescript supports this?

@knocte
Copy link
Member

knocte commented Feb 27, 2023

Let's rebase this PR to get rid of the red CI.

@tehraninasab tehraninasab force-pushed the properRevertMessage-squashed-and-rebased branch from 50de12e to 49650eb Compare February 27, 2023 09:01
knocte
knocte reviewed Feb 27, 2023
View reviewed changes
commitlint.config.ts Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 27, 2023
View reviewed changes
commitlint/helpers.ts Outdated Show resolved Hide resolved
@tehraninasab tehraninasab force-pushed the properRevertMessage-squashed-and-rebased branch 3 times, most recently from 78d319a to 639f132 Compare February 27, 2023 10:20
@knocte knocte force-pushed the master branch 2 times, most recently from d1bd1c2 to 908c11d Compare June 2, 2023 08:09
@knocte knocte force-pushed the master branch 2 times, most recently from c5fb9d5 to 18898b7 Compare July 27, 2023 02:00
@tehraninasab
commitlintplugings.test: add failing tests
db4c07b 
Add failing tests for default-revert-message rule.
@tehraninasab
commitlint.config: add rule
0269352 
Add default-revert-message rule.
@tehraninasab
commitlint/plugins.test: add another test
09d1e26
@tehraninasab tehraninasab force-pushed the properRevertMessage-squashed-and-rebased branch from 639f132 to db4c07b Compare August 9, 2023 13:54
@tehraninasab tehraninasab force-pushed the properRevertMessage-squashed-and-rebased branch from a5cb5c6 to 81f50e3 Compare August 9, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knocte knocte knocte left review comments

Assignees

@tehraninasab tehraninasab

Labels
medium-priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tehraninasab @knocte