-
Notifications
You must be signed in to change notification settings - Fork 943
feat: add rule for BREAKING CHANGE:
in header
#3838
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: add rule for BREAKING CHANGE:
in header
#3838
Conversation
`BREAKING CHANGE:` is a footer thing Fixed conventional-changelog#3810
Test fails for some reason, and I don't know how to debug it. console.log
doesn't work in this context.
Also, subject-empty
triggers instead of this rule, I also don't know how to fix that.
$ echo "BREAKING CHANGE: no" | yarn run commitlint
yarn run v1.22.21
$ /workspace/commitlint/node_modules/.bin/commitlint
⧗ input: BREAKING CHANGE: no
✖ subject may not be empty [subject-empty]
✖ type may not be empty [type-empty]
✖ found 2 problems, 0 warnings
i Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
BREAKING CHANGE: a commit that has a footer BREAKING CHANGE:, or appends a ! after the type/scope, introduces a breaking API change (correlating with MAJOR in Semantic Versioning). A BREAKING CHANGE can be part of commits of any type.
https://www.conventionalcommits.org/en/v1.0.0/#specification
According to the specs BREAKING CHANGE
should be part of the commit-footer, not in the "first line".
The tests here fail correctly I believe because BREAKING CHANGE: is a footer thing
is not a valid commit according to conventionalcommits.
Does this make sense?
I usually use it like this: 5b4aeaf
Update:
Sorry for only giving this feedback now. Should have added this to #3810. Maybe I'm reading the specs wrong?
@escapedcat in the test https://github.com/conventional-changelog/commitlint/actions/runs/7404572868/job/20146281125?pr=3838
Expected: false
Received: true
But it should have received false
. And when I run from command line, the test doesn't trigger at all. And console.log
doesn't work.
So how to develop this without debug info?
I think my main point was that BREAKING CHANGE: is a footer thing
should not work according to the spec
@escapedcat, so the test for BREAKING CHANGE:
as the first line should return false
, right?
@escapedcat, so the test for
BREAKING CHANGE:
as the first line should returnfalse
, right?
I think so, yes. It's only valid to add BREAKING CHANGE:
to the commit-footer, i.e. see this example
Should have noted that when you opened #3810 but didn't notice it back then.
@escapedcat right, and the Jest test expects the test to return false
, but the test returns something different and I don't know how to debug it (first time writing TypeScript).
Not sure when I have time to look into it.
You can try to output the result like this maybe:
console.log(JSON.stringify(YOUR_JEST_RESULT))
console.log
works no problem - I misread Jest output. 🤦
When I added console.log
here.
const result = parsed.subject?.startsWith('BREAKING CHANGE:');
console.log(result);
I got this error.
FAIL @commitlint/rules/src/subject-breaking.test.ts
●くろまる Console
console.log
undefined
at log (@commitlint/rules/src/subject-breaking.ts:6:10)
And thought that console.log
is undefined
, but it is result
which is undefined
.
Now the most interesting part - parsed.subject
is null
.
The specification doesn't define subject
at all - https://www.conventionalcommits.org/en/v1.0.0/#specification
{
type: null,
scope: null,
subject: null,
merge: null,
header: 'BREAKING CHANGE: this one',
body: null,
footer: null,
notes: [],
references: [],
mentions: [],
revert: null,
raw: 'BREAKING CHANGE: this one'
}
I should be checking header
field here.
I send PR to simplify the specification text conventional-commits/conventionalcommits.org#559
There is still no subject
and header
definitions in the spec - it says description
instead of subject
, but that change would be too big, and I am not sure it will be accepted.
Tests are passing, but I still can't trigger the rule from the command line.
$ echo "BREAKING CHANGE: no" | ./@commitlint/cli/cl
i.js
⧗ input: BREAKING CHANGE: no
✖ subject may not be empty [subject-empty]
✖ type may not be empty [type-empty]
✖ found 2 problems, 0 warnings
i Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
I assume the issue here is that it doesn't find a subject because the type already failes?
commitlint assumes BREAKING CHANGE:
is the type
which isn't a valid type
?
Not 100% sure though.
A check needs to happen before they type check maybe?
A check needs to happen before they type check maybe?
That's what I thought too. How to do that?
From my very short understanding of commitlint so far, what might be happening here is that when commitlint's parser finds "BREAKING CHANGE:" at the beginning of the header, in theory it should try to check if the string "BREAKING CHANGE" is part of the allowed types in the configuration, however, the regExp to extract the "potential" type (the string before the colon) might not be prepared to handle spaces, therefore it already fails to parse it.
@knocte the regexp checks header
content, not type
.
Ah true, but I was actually (mistakenly) referring to subject: if type cannot be parsed, then subject cannot either.
BREAKING CHANGE:
in subject (削除ここまで)BREAKING CHANGE:
in header (追記ここまで)
I changed the issue title to avoid subject/header confusion. It is still not clear why it doesn't trigger.
Thanks people! We good here now?
@escapedcat no, it is not clear how to debug why the rule doesn't trigger.
Alright, let me switch this to draft for now
Tests are passing, but I still can't trigger the rule from the command line.
$ echo "BREAKING CHANGE: no" | ./@commitlint/cli/cl i.js ⧗ input: BREAKING CHANGE: no ✖ subject may not be empty [subject-empty] ✖ type may not be empty [type-empty] ✖ found 2 problems, 0 warnings i Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
I can confirm your new rule when adding header-breaking
rule to @commitlint/config-conventional/src/index.ts.
export default {
parserPreset: "conventional-changelog-conventionalcommits",
rules: {
"body-leading-blank": [RuleConfigSeverity.Warning, "always"] as const,
"body-max-line-length": [RuleConfigSeverity.Error, "always", 100] as const,
"footer-leading-blank": [RuleConfigSeverity.Warning, "always"] as const,
"footer-max-line-length": [
RuleConfigSeverity.Error,
"always",
100,
] as const,
+ "header-breaking": [RuleConfigSeverity.Error, "always"] as const,
"header-max-length": [RuleConfigSeverity.Error, "always", 100] as const,
"header-trim": [RuleConfigSeverity.Error, "always"] as const,
The result I got is below with echo "BREAKING CHANGE: this one" | ./@commitlint/cli/cli.js
command.
⧗ input: BREAKING CHANGE: this one ✖ move BREAKING CHANGE: to footer [header-breaking] ✖ subject may not be empty [subject-empty] ✖ type may not be empty [type-empty] ✖ found 3 problems, 0 warnings i Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
Therefore, I think this PR is almost good.
It will be perfect if you add header-breaking
document and typescript types.
@abitrolly Could you continue to update the PR?
Description
BREAKING CHANGE:
is a footer thingFixed #3810
Motivation and Context
Usage examples
How Has This Been Tested?
Types of changes
Checklist: