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

Case sensitivity #110

Closed
zeke opened this issue Dec 17, 2018 · 23 comments
Closed

Case sensitivity #110

zeke opened this issue Dec 17, 2018 · 23 comments

Comments

@zeke
Copy link
Member

zeke commented Dec 17, 2018

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: zeke/semantic-pull-requests#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
Copy link

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

pvdlg commented Dec 18, 2018

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

stevemao commented Dec 18, 2018

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

tunnckoCore commented Dec 18, 2018

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

zeke commented Dec 18, 2018

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

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

zeke commented Dec 20, 2018

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

tunnckoCore pushed a commit to tunnckoCoreLabs/parse-commit-message that referenced this issue Dec 20, 2018
tunnckoCore pushed a commit to tunnckoCoreLabs/parse-commit-message that referenced this issue Dec 20, 2018
* fix: case insensitive header regex
* fix: case insensitivity for the `increment` plugin

Updates for the zeke/semantic-pull-requests#39 and conventional-commits/conventionalcommits.org#110

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>
@tunnckoCore
Copy link
Member

My side is done. :) zeke/semantic-pull-requests#39 should be fixed too.

@bcoe
Copy link
Member

bcoe commented Dec 29, 2018

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

bcoe commented Dec 29, 2018

anyone feel like making a pull?

@damianopetrungaro
Copy link
Member

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

@zeke
Copy link
Member Author

zeke commented Dec 30, 2018

@damianopetrungaro no I haven't touched anything.

tunnckoCore pushed a commit to tunnckoCore/monorepo that referenced this issue Jan 15, 2019
* fix: case insensitive header regex
* fix: case insensitivity for the `increment` plugin

Updates for the zeke/semantic-pull-requests#39 and conventional-commits/conventionalcommits.org#110

Signed-off-by: Charlike Mike Reagent <mameto2011@gmail.com>
@bcoe
Copy link
Member

bcoe commented Feb 18, 2019

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

zeke commented Feb 18, 2019

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

tunnckoCore commented Feb 18, 2019

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

hutson commented Feb 19, 2019

@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 👍 )

perhaps we update the spec to

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

@bcoe
Copy link
Member

bcoe commented Mar 16, 2019

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

@bcoe
Copy link
Member

bcoe commented Mar 16, 2019

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

@stevemao
Copy link
Member

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

@tunnckoCore
Copy link
Member

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

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

@bcoe
Copy link
Member

bcoe commented Mar 25, 2019

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

conventional-changelog/conventional-changelog#429

@bcoe bcoe closed this as completed Mar 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants