-
Notifications
You must be signed in to change notification settings - Fork 951
feat(rules): add breaking-change-exclamation-mark #4548
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
feat(rules): add breaking-change-exclamation-mark #4548
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| return [true]; | ||
| } | ||
|
|
||
| const hasExclamationMark = !!header && /!:/.test(header); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make this change.
Should this be updated to use the breakingHeaderPattern too? To me, that is outside of the scope of this pull request and should be a separate commit.
| const hasExclamationMark = /!:/.test(input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think such tiny change is fine to be inclined in one PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make the change.
| } | ||
|
|
||
| const hasExclamationMark = !!header && /!:/.test(header); | ||
| const hasBreakingChange = !!footer && /BREAKING[ -]CHANGE:/.test(footer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need make sure the footer line starts with BREAKING not only includes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I will modify the regex for a multiline search.
| import message from "@commitlint/message"; | ||
| import { SyncRule } from "@commitlint/types"; | ||
|
|
||
| export const breakingChangeExclamationMark: SyncRule = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new rule should be added into
commitlint/@commitlint/types/src/rules.ts
Line 92 in 0735b63
| export type RulesConfig<V = RuleConfigQuality.User> = { |
@commitlint/rules/src/index.ts
Outdated
| import { typeMinLength } from "./type-min-length.js"; | ||
|
|
||
| export default { | ||
| "breaking-change-exclamation-mark": breakingChangeExclamationMark, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we resort them alphabetically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I will fix the sorting.
docs/reference/rules.md
Outdated
| @@ -1,5 +1,19 @@ | |||
| # Rules | |||
|
|
|||
| ## breaking-change-exclamation-mark | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above about the order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed again; I will fix this. It has bothered me in the past looking things up on this page as it is about 90% in the right order.
4ba9f06 to
28e06e1
Compare
28e06e1 to
bfab842
Compare
|
I made several pushes because I resolved a conflict with a merge instead of performing a rebase 🤦. I have implemented all suggestions and rebased the branch onto the latest master, so I believe it is ready now. |
|
Thanks @adamchristiansen ! |
Description
A breaking change can be denoted by either
!in the header (e.g.,feat!: add advanced search filters) or by includingBREAKING CHANGEin the footer (e.g.,BREAKING CHANGE: old queries no longer work with the new API.). This pull request adds thebreaking-change-exclamation-markrule that requires aBREAKING CHANGEfooter if!appears in the header and a!in the header ifBREAKING CHANGEappears in the footer. The idea is that it is all or nothing, so if one breaking change marker is present the other must be present too.Motivation and Context
Implements and closes #4547.
This feature enforces a solution to three problems:
!might not convey enough information. For example,feat!: add advanced search filters. Why is this a breaking change? How do I know what broke unless the footer is present? Using this rule will fail because a footer indicating a breaking change must be added in this case.BREAKING CHANGEmight cause confusion. For example, if I use something likegit log --onlineto quickly look at changes and I seefeat: add advanced search filters, I have no idea that it is a breaking change without looking at that specific commit in greater detail. This rule will fail because an exclamation mark must be added to the head in this case.Usage examples
How Has This Been Tested?
I added a new test
@commitlint/rules/src/breaking-change-exclamation-mark.test.tsthat follows the structure of existing tests. The commit messages to tests were decided based on equivalence partitions.Types of changes
Checklist: