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 #32

Closed

Conversation

tehraninasab
Copy link
Contributor

No description provided.

@knocte
Copy link
Member

knocte commented Nov 15, 2022

@realmarv CI is red



test('proper-revert-message3', () => {
let commitMsgWithProperRevertMessage = 'Revert "add abbreviations.ts"';
Copy link
Member

Choose a reason for hiding this comment

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

ah damn I'm reading test3 not test2, sorry, let me read test2

@knocte
Copy link
Member

knocte commented Nov 17, 2022

Ignore all my comments above, let's rebase this PR first.

@tehraninasab
Copy link
Contributor Author

Ignore all my comments above, let's rebase this PR first.

Done

commitlint.config.ts Outdated Show resolved Hide resolved
@tehraninasab
Copy link
Contributor Author

tehraninasab commented Nov 17, 2022 via email

@knocte
Copy link
Member

knocte commented Nov 17, 2022

Should it be without a body?

Yes, just a title that says "Revert .NET6 upd as it broke CI"

@tehraninasab
Copy link
Contributor Author

"Revert .NET6 upd as it broke CI"

Done

commitlint.config.ts Outdated Show resolved Hide resolved
@tehraninasab tehraninasab force-pushed the properRevertMessage-new branch 3 times, most recently from 1b6d4ca to fd72e45 Compare November 17, 2022 11:06
if (body !== null) {
let bodyStr = convertAnyToString(body, "body").trim();
let lines = bodyStr.split('\n');
offence = lines[0].match(/^This reverts commit [^ ]{40}.$/) !== null && lines.length == 1;
Copy link
Member

Choose a reason for hiding this comment

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

@realmarv are you sure you don't need to escape the '.' here? AFAIU, dots in regexes act as any character, but here I think you're trying to match it with a real dot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@knocte
Copy link
Member

knocte commented Jan 8, 2023

You didn't push your branch with push1by1. Also, this PR needs a rebase.

@tehraninasab
Copy link
Contributor Author

You didn't push your branch with push1by1. Also, this PR needs a rebase.

Done

},
// Commitlint automatically ignores some kinds of commits like Revert commit messages.
Copy link
Member

Choose a reason for hiding this comment

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

2 last things to do in this PR:

  • (Nit) Add an EOL before this comment above, to visually separate it better from the rule configuration list.
  • Remove the TODO comment related to this PR, at line 363.

@tehraninasab
Copy link
Contributor Author

This PR is closed and replaced with #68 because some commits needed to be squashed and rebased.

@knocte
Copy link
Member

knocte commented Feb 10, 2023

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

@knocte
Copy link
Member

knocte commented Feb 10, 2023

Oops, this one is closed, sorry.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants