-
Notifications
You must be signed in to change notification settings - Fork 946
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
base: master
Are you sure you want to change the base?
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> = { |
Addresses review feedback from @JounQin about pull request conventional-changelog#4548. - Use `breakingHeaderPattern` to search for the exclamation mark in the header. - Correct the regular expression to require that BREAKING CHANGE in the footer be anchored at the beginning of a line. - Updated the `RulesConfig` type.
@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
Implements and closes conventional-changelog#4547.
Addresses review feedback from @JounQin about pull request conventional-changelog#4548. - Use `breakingHeaderPattern` to search for the exclamation mark in the header. - Correct the regular expression to require that BREAKING CHANGE in the footer be anchored at the beginning of a line. - Updated the `RulesConfig` type.
Address pull request feedback from @JounQin. - Fixed regex for `subject-exclamation-mark`. - Fixed sorting for `RulesConfig`. - Fixed sorting for `rules.md`.
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. |
Description
A breaking change can be denoted by either
!
in the header (e.g.,feat!: add advanced search filters
) or by includingBREAKING CHANGE
in the footer (e.g.,BREAKING CHANGE: old queries no longer work with the new API.
). This pull request adds thebreaking-change-exclamation-mark
rule that requires aBREAKING CHANGE
footer if!
appears in the header and a!
in the header ifBREAKING CHANGE
appears 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 CHANGE
might cause confusion. For example, if I use something likegit log --online
to 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.ts
that follows the structure of existing tests. The commit messages to tests were decided based on equivalence partitions.Types of changes
Checklist: