conventional-changelog / commitlint

📓 Lint commit messages
https://commitlint.js.org
MIT License
16.41k stars 883 forks source link

subject-case rule breaks ConventionalCommits spec #2141

Open mrmartan opened 3 years ago

mrmartan commented 3 years ago

@commitlint/config-conventional subject-case rule breaks ConventionalCommits spec

Expected Behavior

From https://www.conventionalcommits.org/en/v1.0.0/#specification

  1. The units of information that make up Conventional Commits MUST NOT be treated as case sensitive by implementors, with the exception of BREAKING CHANGE which MUST be uppercase.

Current Behavior

⧗   input: fix: Gitlab CI pipeline fixed
See the TP for details
Refs #131167
✖   subject must not be sentence-case, start-case, pascal-case, upper-case [subject-case]
✖   found 1 problems, 0 warnings
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

Affected packages

Possible Solution

Remove the casing rule

escapedcat commented 3 years ago

Looks like this should be updated: https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional#subject-case

For now you could change the subject-case rule in your own config.

iamscottcab commented 3 years ago

I this true also of type-case and scope-case? They are both currently set to lowerCase but in theory TYPE(scope): sUbJeCt is a valid commit?

Is the wording implying that type, scope and subject are all individual units within the overall commit, or are units referring to elements in the subject and body respectively?

escapedcat commented 3 years ago

[...] in theory TYPE(scope): sUbJeCt is a valid commit?

I guess so? Although all their examples are showing non-mixed-cases. Would it be possible to allow Subject and subject but not sUbJeCt with the current existing rules? Need to check.

Is the wording implying that type, scope and subject are all individual units within the overall commit, or are units referring to elements in the subject and body respectively?

If it's referring to this part I think it means everything? type, scope, subject, body and footer?

iamscottcab commented 3 years ago

I guess my point being that (ignoring the silly spongebob case) should we be enforcing lowerCase on type and scope or should the rule just be ignored to let the user input the case however they want?

escapedcat commented 3 years ago

The units of information that make up Conventional Commits MUST NOT be treated as case sensitive

I guess that's a yes for "the user input the case however they want".
But then showing lower-case only examples only..... . .. . . . ....

I feel like we should enforce some sort of consistency otherwise people will end up with changelogs saying, i.e.:

  • Fix(button): Changed color
  • feat(Select): changed padding
    • Docs(settings): ChangED All tHE seTTinGS

That can't be what people would expect from commitlint, right?
@mrmartan what do you think/expect?

iamscottcab commented 3 years ago

Agreed, I think the default config should set some standard, as you mentioned above you can tailor the plugins to behave in a certain way. I think we should enforce lowerCase on type and scope at the very least. But I figure this issue raises a sort of meta discussion so might be worth deciding how much of the config we change if / before we do :)

escapedcat commented 3 years ago

@mrmartan we tend to close this.
commitlint should enforce certain cases. 100% according to spec would lead to inconsistent messages. If people want to be 100% according to spec when it comes to cases they can use their own commitlint-config.

mrmartan commented 3 years ago

I tend to disagree. I see your point but raise that with the spec itself. All implementors are expected to conform to it. Otherwise problems arise in heterogenous environments. That's in fact how we arrived here. One tool in one part of the system would be ok with fix(button): Changed color and another tool (this one) would not. Now you have to start matching configuration of multiple tools that only sort of conform to a standard.

At least make the casing rules opt-in instead of opt-out.

As for consistency, the spec itself argues for consistency but does not presume what that should be. https://www.conventionalcommits.org/en/v1.0.0/#are-the-types-in-the-commit-title-uppercase-or-lowercase

There is a more demanding solution. You could pickup the preferred style from the commits themselves and make them self-consistent on contrary to all lower-case. I can imagine the inconsistencies, in type and scope in particular, could cause trouble down the line if other plugins/tool are not case-insensitive, e.g. release notes generators.

mrmartan commented 3 years ago

So I have tried disabling the case-sensitvity rules. The type-enum rule (evaluation implementation) itself is also case-sensitive. Can you see the issue?

iamscottcab commented 3 years ago

We've been having a bit of a discussion about this one in Slack and can appreciate where you are coming from. I think (internally) there is a consensus for us to provide some basic and generally adopted opinionated defaults, as we do now, based on the conventional changelog format.

However we do appreciate that some users might want something that matches the spec but is less opinionated. I'm not sure how this will turn out in practice yet.

One suggestion was relaxing the rules on the current configuration and also supplying a commitlint configuration which keeps the values we ship currently. I'm partial to this idea because the commitlint config would be for those who want a simple out-of-the-box solution with some rules built in for them that we think are commonly used. Alternatively the conventional config could be adopted for those who want to match the spec 100%.

I guess we would have to discuss how that affects documentation and onboarding because we already get questions about trying to streamline setup and two configs might be confusing for users.

EDIT: @escapedcat a change like this would also be considered a breaking change I guess if we loosen or change config values as current users may expect lowerCase enforcement on type and scope for example?

Thoughts @mrmartan?

mrmartan commented 3 years ago

Well, from my POV @commitlint/config-conventional was from the start about setting up rules conforming to the standard. So when it does not, I see it as a bug and any change to make it conform does not break the original contract. Which strictly speaking means also removing the type-enum rule. I can understand your dilemma but there are many configs available and if one wanted an opinionated one, he could have chosen so. Here you have taken one that refers itself to a standard and made it opinionated.

TimothyJones commented 2 years ago

100% according to spec would lead to inconsistent messages. If people want to be 100% according to spec when it comes to cases they can use their own commitlint-config.

I found this issue because I came here to report that @commitlint/config-conventional enforces a particular case when the spec explicitly does not. I think it's reasonable to expect a config called @commitlint/config-conventional to only enforce rules from the spec.

The conventional commits FAQ does recommend keeping a consistent case:

Any casing may be used, but it’s best to be consistent.

I like the point that people using commitlint expect extra consistency - what about having two rule sets:

// Just the rules according to the spec
@commitlint/config-conventional 

// The rules plus some extra consistency
@commitlint/config-conventional-recommended  

Edit: I'm happy to put together a PR if you like the two rulesets approach.

escapedcat commented 2 years ago

@TimothyJones hey thanks for your offer.
@AdeAttwood I remember we talked about this. What was the idea you suggested?

AdeAttwood commented 2 years ago

It depends on how many rules we are making opinions on. If it's just this one, then I think It's OK to enforce this rule and let the user override it if they want to use a different case.

From the comments of

Any casing may be used, but it’s best to be consistent.

We are adding a rule, so there must be a rule for the case of the message, and setting it to lower-case. If the user wants to use another case, they can override the rule in their config to change it. That way, we are enforcing a rule for consistency, not case. Which while reading this back could be considered and opinionated opinion.

I think it's reasonable to expect a config called @commitlint/config-conventional to only enforce rules from the spec.

I think this could be a bigger issue and if we are doing down the road of 100% spec, we work with the team making the spec to come up with a test suite like json-patch and then any project can implement that test suite to comply to the spec.

TimothyJones commented 2 years ago

For me, the issue is that we don’t want to bikeshed the rules within the team, we want to just adopt the ones from conventional-commits. It would be nice if we can do that by using an existing config. So, we found the @commitlint/config-conventional package.

However, that package implements the conventional commits rules, plus some extra ones.

Pragmatically, as you point out, we can override those bits, but this means it’s less easy to have the same config everywhere (unless we publish actually-config-conventional, which feels unnecessary, since the “best” package name is the exisiting one).

It’s also concerning as a user- because if the rules here are the spec plus some modifications, where else do the rules differ, and how do we know?

It’s much easier to trust a lint package named after a specification if it only implements the rules from that specification.

piercy commented 11 months ago

My team recently adopted this and are finding pain that the package doesn't follow conventional commits specification. We will be adding overrides. As this package advertises itself as following the spec, it probably shouldn't get opinionated about these things. Specifically casing and length were the issues for us.

Zordrak commented 3 months ago

An obvious use case that confirms to the spec, but is rejected by this implementation:

type(scope): JIRAISSUE-123: short description of change that resolves JIRAISSUE-123

I fully agree with previous commenters that while commitlint has every right to provide an opinionated stance, such as via a config called @commitlint/config-conventional-recommended, it's important that there is a config that applies the spec and only the spec, and an ordinary person's assumption would be that @commitlint/config-conventional is the config that enforces the spec.

piercy commented 3 months ago

Realistically, I don't think we're ever going to see this fixed. It's been 3.5 years and is still open. The current maintainers are happy to advertise the factually correct but drastically misleading statement below, while also leaving this ticket open knowing they have added their own rules in.

Anyone following this shouldn't really hold their breath. It will be faster and easier to implement your own rules to move it close to the spec, than it will be to wait for this package to follow the spec.

commitlint checks if your commit messages meet the conventional commit format.