conventional-commits / conventionalcommits.org

The conventional commits specification
https://conventionalcommits.org
MIT License
6.73k stars 520 forks source link

Case sensitivity #110

Closed zeke closed 5 years ago

zeke commented 5 years ago

There seems to be no mention of case (in)sensitivity in the spec. Should there be?

Asking because I ran into this today and I'm not sure how to resolve it: https://github.com/probot/semantic-pull-requests/issues/39

@pvdlg does semantic-release require lowercase?

@greysteil is there a particular reason you've chose capitalized PR titles for @dependabot?

cc @tunnckoCore, maintainer of parse-commit-message, which appears to be case sensitive. (lowercase required).

greysteil commented 5 years ago

From Dependabot's side, I think those capitalisations are a mistake. I don't know when the bug was introduced, but at some point a couple of weeks ago I think a version of Dependabot shipped that accidentally capitalised the prefix.

Unfortunately that bug has been compounded by the updated Dependabot logic here, which means Dependabot will continue to use whatever prefix it last used in a merged PR - in your case that is now capitalised.

If you update the commit message on a Dependabot commit when merging it (e.g., using GitHub's squash and merge) then Dependabot should pick that up and start using that prefix instead.

pvdlg commented 5 years ago

Yes semantic-release is case sensitive. I'm not sure why the decision was made. I probably didn't think about that case. It hasn't been a problem so far. However the default for semantic-release is the angular convention which specifies each type in lower case, without clearly mentioning that the case matter. My personal reading of the Angular convention is the type must be lower case.

stevemao commented 5 years ago

I think the spec should be a bit strict (case sensitive). But the tools can be a bit loose (case insensitive).

The angular team asked me to allow BREAKING CHANGE: to be case insensitive or at least configurable. So did the jshint team. There are (old) commits in those repos unfortunately have different casing (mostly made by casual contributors when they don't use tools like commitlint).

tunnckoCore commented 5 years ago

I noticed that tool, before few weeks. And think that all parts should be insensitive except the BREAKING CHANGE: - the type, scope and etc.

Dang, I forgot to set the i to the regex here which actually isn't currently a problem.

Yea, the problem comes from the increment plugin, but you always can create some similar plugin and use it instead of this one.

zeke commented 5 years ago

I think the spec should be a bit strict (case sensitive). But the tools can be a bit loose (case insensitive).

☝️ This sounds good to me. Anyone want to take a crack at adding this to the spec?

dijonkitchen commented 5 years ago

This was discussed in #24, but I have found a good justification for lowercase if you are a fan of Google's reasoning for Go/Angular: https://golang.org/doc/contribute.html#commit_messages

zeke commented 5 years ago

Thanks for sharing that old issue, @dijonkitchen. I searched but somehow missed it.

Hmm, for some reason that FAQ entry about uppercase vs lowercase is not showing up on the website, even though the website appears to be serving v1.0.0-beta.2 of the spec. @damianopetrungaro, you've been the most active deployer lately -- any idea what might be going on there? 🤔

From the above link:

A rule of thumb is that it should be written so to complete the sentence "This change modifies Go to _____." That means it does not start with a capital letter, is not a complete sentence, and actually summarizes the result of the change.

I like that. It works well with the GitHub convention of using imperative verb forms like change foo instead of changes foo or changed foo

All that aside, it seems this topic has already been discussed and a decision was made. Given that the Semantic Pull Requests bot is designed to play nicely with semantic-release which is case sensitive (lowercase required), I will probably update the bot to fail checks that are uppercase, but provide a helpful status message to indicate when the message would be valid if it was lowercase...

dijonkitchen commented 5 years ago

Yeah, seems to be in the "next" version of the content: https://github.com/conventional-commits/conventionalcommits.org/blob/master/content/next/index.md

tunnckoCore commented 5 years ago

My side is done. :) https://github.com/probot/semantic-pull-requests/issues/39 should be fixed too.

bcoe commented 5 years ago

👋 I'm late to the party, but I'd always assumed that the spec expected lower-case (except for BREAKING CHANGE which expects all upper-case); I agree that we should call this out explicitly on the website?

bcoe commented 5 years ago

anyone feel like making a pull?

damianopetrungaro commented 5 years ago

@zeke did you update the next version or the current beta?

zeke commented 5 years ago

@damianopetrungaro no I haven't touched anything.

bcoe commented 5 years ago

@zeke @tunnckoCore @damianopetrungaro where did folks land on this? do we not care about case sensitivity, with the exception of "BREAKING CHANGE" being all upper case?

This is potentially more user-friendly, and I'd be fine with this approach ... perhaps we update the spec to:

1. Commits MUST be prefixed with a type, which consists of a noun, feat, fix, etc., followed by a colon and a space (types are not case sensitive).

@stevemao ☝️would this require sweeping changes to the parser?

zeke commented 5 years ago

I'd still lean toward a lowercase requirement, but that doesn't seem to be the popular opinion here.

semantic-release requires lowercase prefixes, so if the final decision for the spec is to remain case insensitive , I may have to make https://github.com/probot/semantic-pull-requests deviate slightly from the spec to maintain compatibility with semantic-release.

tunnckoCore commented 5 years ago

@bcoe: do we not care about case sensitivity, with the exception of "BREAKING CHANGE" being all upper case?

Exactly.

I don't think spec should care and it should be lite/easy on that (so, insensitive). Tools and authors on top of it can decide whatever they want as with the case with semantic-release. :) That's why I made it (parse-commit-message) insensitive too, because I'm lower-level tool/package.

That way @zeke's semantic-pull-requests won't "deviate slightly from the spec" - it still will be spec compliant. :) So, win-win.

hutson commented 5 years ago

@bcoe I believe the parser just relies on the convention package.

I believe some, if not all, convention packages are case insensitive.

Though, it probably doesn't matter if, say, angular required upper case, because that's their decision to make with respect to their convention. (I would love to see a @conventional-commits/conventional-changelog-config configuration package that embodies the conventions in this spec :+1: )

perhaps we update the spec to

In addition to, or in place of, the FAQ item that exists?

bcoe commented 5 years ago

@stevemao can you confirm that the toolchain is case insensitive?

bcoe commented 5 years ago

☝️ @stevemao if so, I suggest we add this to the specification for upstream tooling implementors.

stevemao commented 5 years ago

BREAKING CHANGE is, but feat, fix, etc are not ATM. It's very easy to change though.

tunnckoCore commented 5 years ago

I think that we can add one more point - the 12 which will say something like:

"All parts of the commit message are case insensitive, except the BREAKING CHANGE one. Note that, upstream implementers of this specification MAY enforce case sensitivity for some of the parts - for example, the type to be always lowercased."

I'm all in for case insensitive in the parsers side (like my case with parse-commit-message parser), so higher level tools and users may enforce whatever they want.

damianopetrungaro commented 5 years ago

I am fine with both actually. I do not see any advantage imposing case sensibility here, exception made for the BREAKING CHANGE:.

bcoe commented 5 years ago

we've added a stanza removing case-sensitivity from the latest revision of the spec :+1: for the record, I'm working on a conventionalcommits preset for conventional-changelog that is true to this decision:

https://github.com/conventional-changelog/conventional-changelog/pull/429