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

Update type-empty rule #3047

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

FelixLgr
Copy link
Contributor

@FelixLgr FelixLgr commented Feb 24, 2022

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

  • 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.
  • I have updated the documentation accordingly.
  • 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 : #3036

@escapedcat
Copy link
Member

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?

@FelixLgr
Copy link
Contributor Author

FelixLgr commented Mar 5, 2022

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'
}

@escapedcat
Copy link
Member

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.

@FelixLgr
Copy link
Contributor Author

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

@mehdicopter
Copy link

hey 👋
will it be merged ?

@escapedcat
Copy link
Member

@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?

@FelixLgr
Copy link
Contributor Author

FelixLgr commented Oct 29, 2022

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

@escapedcat escapedcat marked this pull request as draft October 20, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants