commitlint icon indicating copy to clipboard operation
commitlint copied to clipboard

Using Periods With Start Case In Subject Fails

Open Deadmano opened this issue 3 years ago • 4 comments
trafficstars

Expected Behavior

When using the start-case option in the config for subject-case entering a message such as Feat: Add Module 1.0.0 or Fix: Remove Invalid Return Value In Core.js should pass.

Current Behavior

A problem is found in the current version not allowing for messages with periods to be added unless quoted such as Feat: Add Module "1.0.0" or Fix: Remove Invalid Return Value In "Core.js"

Affected packages

  • [x] cli
  • [x] core

Steps to Reproduce (for bugs)

  1. Set the commitlint.config.js as per the below snippet.
  2. In a terminal enter echo Feat: Add Module 1.0.0 | commitlint -V
  3. Observe the following error output:
â§—   input: Feat: Add Module 1.0.0
✖   subject must be start-case [subject-case]
commitlint.config.js
module.exports = {
  extends: ['@commitlint/config-conventional'],
  rules: {
    'body-case': [2, 'always', 'sentence-case'],
    'header-max-length': [2, 'always', 72],
    'subject-case': [2, 'always', 'start-case'],
    'subject-empty': [2, 'never'],
    'subject-full-stop': [2, 'never', '.'],
    'type-case': [2, 'always', 'start-case'],
    'type-empty': [2, 'never'],
    'type-enum': [
      2,
      'always',
      [
        'Feat',
        'Fix'
      ],
    ],
  },
};

Context

I'd like to be able to use start case subject messages and also be able to include semantic versioning without it failing.

Your Environment

Executable Version
commitlint --version 17.0.3
git --version 2.25.1
node --version 16.14.2

Deadmano avatar Aug 08 '22 00:08 Deadmano

Looks like it can't handle the 1.0.0 in the subject. If I leave that out it works. :(

escapedcat avatar Aug 08 '22 03:08 escapedcat

@escapedcat can you confirm if it works with the subject containing Index.js or any bit of text that has a period in it such as This.Is.A.Test or App.Path() etc? I believe that is the issue.

Deadmano avatar Aug 08 '22 04:08 Deadmano

Yeah, non of this works.

escapedcat avatar Aug 08 '22 04:08 escapedcat

Well, this is quite annoying.

All the case filters check whether the origin and transformed strings are the same:

https://github.com/conventional-changelog/commitlint/blob/f1b9b11c31adae0eb5f17f5e7eb3499f3d663227/%40commitlint/ensure/src/case.ts#L16-L22

And the case conversion simply calls out to lodash: https://github.com/conventional-changelog/commitlint/blob/f1b9b11c31adae0eb5f17f5e7eb3499f3d663227/%40commitlint/ensure/src/to-case.ts#L6 https://github.com/conventional-changelog/commitlint/blob/f1b9b11c31adae0eb5f17f5e7eb3499f3d663227/%40commitlint/ensure/src/to-case.ts#L18-L19

...and lodash normalises to "words" by filtering out all non-word-characters:

https://github.com/lodash/lodash/blob/ddfd9b11a0126db2302cb70ec9973b66baec0975/lodash.js#L14629-L14631 https://github.com/lodash/lodash/blob/ddfd9b11a0126db2302cb70ec9973b66baec0975/lodash.js#L4968-L4972 https://github.com/lodash/lodash/blob/ddfd9b11a0126db2302cb70ec9973b66baec0975/lodash.js#L15218-L15223 (using either reUnicodeWord or reAsciiWord)

Both will consider "foo 1.0" as three words: foo, 1 and 0. When merging the words back together, lodash doesn't care what separated the words, all is normalised to spaces, so we end up getting foo 1.0 => foo 1 0. That seems to be by design, and not an oversight. Their examples show that's the intended use here; they'd want to transform Core.js into Core Js, since those are two separate words, and for general text...that may be correct.

This also affects other per-word sentence casing, like pascal-case or camel-case or similar.

I'm unsure what to suggest here. On the one hand, there may be desire to point out some inaccurate prose, e.g. foo: Fix.and Florg doesn't make much sense. On the other, this is programming, and the edge case is the normal case, since we have versions and file extensions and module.specifiers. Another possible avenue is detailing such a shortcoming and recommending to keep it as a warning. Yet another is deprecating these if we can't make them work.

Zirak avatar Aug 23 '22 09:08 Zirak