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

subject-case rule breaks ConventionalCommits spec #2141

Open
1 task
mrmartan opened this issue Sep 23, 2020 · 18 comments
Open
1 task

subject-case rule breaks ConventionalCommits spec #2141

mrmartan opened this issue Sep 23, 2020 · 18 comments
Assignees

Comments

@mrmartan
Copy link

mrmartan commented Sep 23, 2020

@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

  • commitlint

Possible Solution

Remove the casing rule

@mrmartan mrmartan changed the title subject-case rule breaks ConventionaCommit spec subject-case rule breaks ConventionalCommits spec Sep 23, 2020
@escapedcat
Copy link
Member

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.

@escapedcat escapedcat added the bug label Sep 24, 2020
@escapedcat escapedcat self-assigned this Sep 24, 2020
@iamscottcab
Copy link
Collaborator

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
Copy link
Member

[...] 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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

@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.

@escapedcat escapedcat added discussion and removed bug labels Sep 24, 2020
@mrmartan
Copy link
Author

mrmartan commented Sep 24, 2020

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
Copy link
Author

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
Copy link
Collaborator

iamscottcab commented Sep 24, 2020

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
Copy link
Author

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
Copy link

TimothyJones commented Aug 27, 2021

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
Copy link
Member

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

@AdeAttwood
Copy link
Member

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
Copy link

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.

ncaq added a commit to ncaq/dotfiles that referenced this issue Jun 24, 2022
[subject-case rule breaks ConventionalCommits spec · Issue #2141 · conventional-changelog/commitlint](conventional-changelog/commitlint#2141)
@piercy
Copy link

piercy commented Aug 15, 2023

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.

@escapedcat escapedcat pinned this issue Aug 16, 2023
ice0 added a commit to ice0/synfig that referenced this issue Jan 29, 2024
This will allow to start subjects from uppercase words.
Currently it fails on something like:
```
fix: Gitlab CI pipeline fixed
build: CMake sets wrong Synfig Studio/ETL versions
```

P.S. This change does not violate the standard. Discussion here:
conventional-changelog/commitlint#2141

P.P.S. Puppeteer did the same: https://github.com/puppeteer/puppeteer/pull/8091/files
ice0 added a commit to ice0/synfig that referenced this issue Jan 29, 2024
This will allow to start subjects from uppercase words. Currently it fails on something like:
```
fix: Gitlab CI pipeline fixed
build: CMake sets wrong Synfig Studio/ETL versions
```

P.S. If I understand correctly, this change does not violate the standard.
Discussion here: conventional-changelog/commitlint#2141

P.P.S. Puppeteer did the same:
https://github.com/puppeteer/puppeteer/pull/8091/files
ice0 added a commit to synfig/synfig that referenced this issue Jan 29, 2024
This will allow to start subjects from uppercase words. Currently it fails on something like:
```
fix: Gitlab CI pipeline fixed
build: CMake sets wrong Synfig Studio/ETL versions
```

P.S. If I understand correctly, this change does not violate the standard.
Discussion here: conventional-changelog/commitlint#2141

P.P.S. Puppeteer did the same:
https://github.com/puppeteer/puppeteer/pull/8091/files
@Zordrak
Copy link

Zordrak commented Apr 11, 2024

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
Copy link

piercy commented Apr 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

7 participants