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

Sentence-case is not work properly #3517

Open
1 of 4 tasks
tugaydmrl opened this issue Jan 31, 2023 · 10 comments
Open
1 of 4 tasks

Sentence-case is not work properly #3517

tugaydmrl opened this issue Jan 31, 2023 · 10 comments

Comments

@tugaydmrl
Copy link

tugaydmrl commented Jan 31, 2023

Expected Behavior

module.exports = {
  extends: ["@commitlint/config-conventional"],
  rules: {
    "subject-case": [2, "always", "sentence-case"],
    "type-enum": [
      2,
      "always",
      ["ADD", "FEAT", "FIX", "REFACTOR", "REMOVE", "TEST", "UPDATE"],
    ],
    "type-case": [2, "always", "upper-case"],
  },
};

I have commitlint.config.js like this. According the rules, my subject sentence should start with capitalized word, other words should be all lowercase.

Current Behavior

But when i try to commit with message like "FEAT: Test Commit", it accepts. It should throw an error because this sentence is not compable with sentence-case.

Affected packages

  • cli
  • core
  • prompt
  • config-angular

Steps to Reproduce

1. First step
Add this rule to your configuration: "subject-case": [2, "always", "sentence-case"]
2. Second step
Make a commit that doesn't follow the rule.

commitlint --version

@commitlint/{cli,config-conventional}^17.4.2,

git --version

v2.30

node --version

v16.5.1

@tugaydmrl tugaydmrl added the bug label Jan 31, 2023
@escapedcat
Copy link
Member

Isn't sentence case for the subject correct in this way? Subject is "Test Commit" and that's sentence case?
https://commitlint.js.org/#/reference-rules?id=subject-case

@escapedcat escapedcat added question and removed bug labels Feb 1, 2023
@tugaydmrl
Copy link
Author

Actually, my expectation from the sentence-case was that it only starts the first word of the sentence with a capital letter and forces all the letters of the remaining words to be lowercase. I think we understood the sentence-case differently.

In my opinion, "Test Commit" should wrong and "Test commit" is true form of the sentence-case. I have seen similar situations in my research on the internet.

If sentence-case works fine, what value should I use to make it do what I want?

Thanks for your interest

@escapedcat
Copy link
Member

Got it! Thanks

In my opinion, "Test Commit" should wrong and "Test commit" is true form of the sentence-case. I have seen similar situations in my research on the internet.

I guess you got a point there 👍
Also the docs say "// Sentence case".
Unfortunately we do not have test for this case, but we do for the others...

Switching the label back to "bug"

@knocte
Copy link
Contributor

knocte commented Feb 3, 2023

In my opinion, "Test Commit" should wrong and "Test commit" is true form of the sentence-case

Wouldn't that ban the use of names in the commit title? Like, let's say, King Kong, or whatever. I wouldn't find this interpretation of sentence-case to be very useful, there would be false negatives all the time.

@escapedcat
Copy link
Member

True, good point. Looking at the current implementation it looks like it does what it's supposed to do. Maybe the docs should be more clear about this.
Wondering why there's no test for it.

I'm tempted to close this, what do you think @tugaydmrl . It will be hard to get this right. If the expectation is "correct English sentence" I doubt it's possible currently.

@escapedcat escapedcat added discussion and removed bug labels Feb 3, 2023
@tugaydmrl
Copy link
Author

I agree with you. Forcing only the first letter of the first word to be capitalized and the others to be lowercase also seems not appropriate for the sentence-case. For example, if New York is included in the sentence, this will also cause a problem. The approach to only capitalize the first letter of the first word is the best approach we have. We can close issue 👍 .

Thanks for your feedbacks guys.

@knocte
Copy link
Contributor

knocte commented Feb 5, 2023

So, is sentence-case Supposed To Be Like This? If so, I would rather rename it to title-case, to avoid further confusions.

@escapedcat
Copy link
Member

escapedcat commented Feb 5, 2023

Deepdived into some history of this feature:

  1. Was introduced here:
    feat(core): add sentence-case to ensure-case #117
  2. Updated here:
    fix: sentence-case doesn't allow upper-case characters in first word #531
  3. Again:
    refactor(cz-commitlint,prompt): remove duplication in case conversion #2794

Not actually sure what to do with this now :P
Looks like it got relaxed and aligned with cz-commitizen over time. Which is "first letter is upperace" and nothing else?

@knocte
Copy link
Contributor

knocte commented Feb 5, 2023

refactor(cz-commitlint,prompt): remove duplication in case conversion #2794

A refactor PR that changed behaviour? 😱

@escapedcat
Copy link
Member

A refactor PR that changed behaviour? 😱

Well... "things" happen. Let's focus on the future here :P

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

No branches or pull requests

3 participants