commitlint icon indicating copy to clipboard operation
commitlint copied to clipboard

Update type-empty rule

Open FelixLgr opened this issue 3 years ago • 7 comments

Description

I have tried to improve type-empty rule test.

Motivation and Context

This PR is necessary to highlight limits posed by the type-empty rule (check this issue

Usage examples

// commitlint.config.js
module.exports = {
    extends: ['@commitlint/config-conventional'],
    rules: {
        'type-empty': [2, 'always'],
    },
};
echo "only subject" | commitlint # passes
echo ": with separator | commitlint # fails

Types of changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [ ] My change requires a change to the documentation.
  • [x] I have updated the documentation accordingly.
  • [x] I have added tests to cover my changes.
  • [ ] All new and existing tests passed.

Notes:

It's the first time I work for a project that is not mine and especially on a library. I only added tests (which may not pass the CI). I'm not sure how everything else works so if I can get some hints or even help I'm not against it. Issue source : https://github.com/conventional-changelog/commitlint/issues/3036

FelixLgr avatar Feb 24 '22 20:02 FelixLgr

Maybe try to start logging things in here: https://github.com/conventional-changelog/commitlint/blob/master/%40commitlint/rules/src/type-empty.ts

See what you find. Does this help?

escapedcat avatar Feb 25 '22 02:02 escapedcat

at first sight, the problem would come from the "conventional-commits-parser". Here is its return

parsed {
  type: null,
  scope: null,
  subject: 'subject',
  merge: null,
  header: ': subject',
  body: null,
  footer: null,
  notes: [],
  references: [],
  mentions: [],
  revert: null,
  raw: ': subject'
}

FelixLgr avatar Mar 05 '22 14:03 FelixLgr

at first sight, the problem would come from the "conventional-commits-parser". Here is its return

Hm, I had a look briefly but wasn't sure.
Maybe have a look here: https://github.com/conventional-commits/parser
If this will always assume there needs to be a type then we can't do anything.
In the end this is still a lib to support conventional (or similar) commits.
We rely on that parser.
Tbh maybe it's easier to make this transparent and update the README regarding this.

What do you think? Won't help with your usecase but at least we're being transparent about what commitlint is trying to achieve/support.

escapedcat avatar Mar 06 '22 03:03 escapedcat

I agree with you. And indeed it would be transparent so I think we can go for that method

FelixLgr avatar Mar 18 '22 10:03 FelixLgr

hey 👋 will it be merged ?

mehdicopter avatar Oct 28 '22 13:10 mehdicopter

@FelixLgr is this ready? What about the commented out tests?
Wasn't sure if that's supposed to be that way I guess. If yes, would you mind explaininig it again and adjist the comment?

escapedcat avatar Oct 28 '22 15:10 escapedcat

I admit I'm not really a fan of adding commented tests (they can be taken as dead code) but actually it's the opposite of what is intuitive. I don't necessarily want to add tests that don't pass either. How can we do it?

As a reminder, the problem is the following: with type-empty:

echo 'Hello world' | commitlint #work (it's normal)
echo ': Hello world' | commitlint #work (not normal, type is `null` and `scope` too)

parser return:

parsed {
      type: null,
      scope: null,
      subject: 'subject',
      merge: null,
      header: ': subject',
      body: null,
      footer: null,
      notes: [],
      references: [],
      mentions: [],
      revert: null,
      raw: ': subject'
    }

I'll work on it again to try to solve the problem. I did it a little too fast

FelixLgr avatar Oct 29 '22 10:10 FelixLgr