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

feat(rules): add header-trim rule #3871

Merged
merged 1 commit into from
Jan 25, 2024
Merged

feat(rules): add header-trim rule #3871

merged 1 commit into from
Jan 25, 2024

Conversation

marcalexiei
Copy link
Contributor

Description

Followup of #3199 (comment):

Currently when having a leading space, the error message says that the subject is empty.
I suggest that the error message instead says that there is leading whitespace.
This is due to identifying extra unwanted leading whitespace is hard in a commit message.

I have added a rule to check that header do not start or end with whitespace characters: header-trim
The rule logs a different status message for each scenario (start / end / mixed).

I added the new rule to @commitlint/config-conventional config.
Let me know if you prefer that the user manually opt-it or it I have to add it to other configurations.

Motivation and Context

#3199

Usage examples

// commitlint.config.js
module.exports = {
  rules: {
    'header-trim': [2, 'always'],
  }
};
# fails
echo " feat: test commit" | yarn run commitlint
echo "feat: test commit " | yarn run commitlint
echo " feat: test commit " | yarn run commitlint

# passes
echo "feat: test commit " | yarn run commitlint
Screenshot 2024-01-25 at 05 02 59

How Has This Been Tested?

yarn test

The new rule has been tested in @commitlint/rules/src/header-trim.test.ts

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.

@escapedcat escapedcat merged commit 331579a into conventional-changelog:master Jan 25, 2024
4 checks passed
@escapedcat
Copy link
Member

Thanks for taking care of this!

@UkonnRa
Copy link

UkonnRa commented Jan 26, 2024

Same as wagoid/commitlint-github-action#755, we met the invalid rule names error as well:

RangeError: Found invalid rule names: header-trim. Supported rule names are: body-case, body-empty, body-full-stop, body-leading-blank, body-max-length, body-max-line-length, body-min-length, footer-empty, footer-leading-blank, footer-max-length, footer-max-line-length, footer-min-length, header-case, header-full-stop, header-max-length, header-min-length, references-empty, scope-case, scope-empty, scope-enum, scope-max-length, scope-min-length, signed-off-by, subject-case, subject-empty, subject-full-stop, subject-max-length, subject-min-length, subject-exclamation-mark, trailer-exists, type-case, type-empty, type-enum, type-max-length, type-min-length
    at lint (/usr/lib/node_modules/@commitlint/cli/node_modules/@commitlint/lint/lib/lint.js:53:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Any idea how to fix it?

@marcalexiei
Copy link
Contributor Author

The error is thrown here:

if (missing.length > 0) {
const names = [...allRules.keys()];
throw new RangeError(
`Found invalid rule names: ${missing.join(
', '
)}. Supported rule names are: ${names.join(', ')}`
);
}

all rules is declared reading the content of baseRules:

const allRules: Map<string, BaseRule<never, RuleType>> = new Map(
Object.entries(defaultRules)
);

baseRules are imported from @commitlint-rules:

import defaultRules from '@commitlint/rules';

in version 14.6.0 header-trim definition has been added

'header-trim': headerTrim,


Running command line from CLI doesn't throw any error.
If I have to make a guess not all required packages have updated to version 18.6.0.

Could you please provide a minimal reproduction example in a separate issue?

@marcalexiei
Copy link
Contributor Author

marcalexiei commented Jan 26, 2024

@UkonnRa
Copy link

UkonnRa commented Jan 26, 2024

You are right, in my case, /usr/bin/node_modules is not the right node_modules dir. For some reason, my commitlint command in husky just use the wrong commitlint command.

We should always use node_modules/.bin/commitlint and make sure the commitlint points to the correct one.
I am OK now.

@kddsultan
Copy link

This is a Breaking change, right? It is not backwards compatible

antoinezanardi pushed a commit to antoinezanardi/werewolves-assistant-web-next that referenced this pull request Jan 27, 2024
## [1.3.0](v1.2.0...v1.3.0) (2024-1-27)

### 🚀 Features

* **about:** about page with every role ([#53](#53)) ([2233d28](2233d28))

### 🧹 Chore

* **deps:** bump @faker-js/faker from 8.3.1 to 8.4.0 ([#68](#68)) ([a6633be](a6633be))
* **deps:** bump @vue/test-utils from 2.4.3 to 2.4.4 ([#69](#69)) ([2bc7faf](2bc7faf))
* **deps:** bump happy-dom from 13.2.0 to 13.2.1 ([#54](#54)) ([a2882de](a2882de))
* **deps:** bump happy-dom from 13.2.1 to 13.3.1 ([#61](#61)) ([1f47fca](1f47fca))
* **deps:** bump husky from 8.0.3 to 9.0.1 ([#62](#62)) ([d5dc87e](d5dc87e))
* **deps:** bump husky from 9.0.1 to 9.0.6 ([#66](#66)) ([41d34e0](41d34e0))
* **deps:** bump primevue from 3.46.0 to 3.47.1 ([#64](#64)) ([ee028ca](ee028ca))
* **deps:** bump primevue from 3.47.1 to 3.47.2 ([#67](#67)) ([0c93a31](0c93a31))
* **deps:** bump the commitlint group with 2 updates ([#58](#58)) ([41637d3](41637d3))
* **deps:** bump the commitlint group with 2 updates ([#65](#65)) ([99f9b90](99f9b90)), closes [conventional-changelog/commitlint#3871](conventional-changelog/commitlint#3871) [#3865](https://github.com/antoinezanardi/werewolves-assistant-web-next/issues/3865) [conventional-changelog/commitlint#3867](conventional-changelog/commitlint#3867) [conventional-changelog/commitlint#3871](conventional-changelog/commitlint#3871) [#3865](https://github.com/antoinezanardi/werewolves-assistant-web-next/issues/3865) [conventional-changelog/commitlint#3867](conventional-changelog/commitlint#3867) [#3199](https://github.com/antoinezanardi/werewolves-assistant-web-next/issues/3199) [#3871](https://github.com/antoinezanardi/werewolves-assistant-web-next/issues/3871) [#3199](https://github.com/antoinezanardi/werewolves-assistant-web-next/issues/3199) [#3871](https://github.com/antoinezanardi/werewolves-assistant-web-next/issues/3871)
* **deps:** bump the types group with 3 updates ([#57](#57)) ([88e5989](88e5989))
* **deps:** bump type-fest from 4.10.0 to 4.10.1 ([#63](#63)) ([eb763ad](eb763ad))
* **deps:** bump type-fest from 4.9.0 to 4.10.0 ([#60](#60)) ([e56c906](e56c906))
* **deps:** bump vue-eslint-parser from 9.4.0 to 9.4.2 ([#59](#59)) ([0990171](0990171))
* **nuxt-image:** downgrade nuxt image because prod problems ([#70](#70)) ([bed26f0](bed26f0))
@escapedcat
Copy link
Member

This is a Breaking change, right? It is not backwards compatible

You might be right. Let's see how many people will be affected.

@tada5hi
Copy link

tada5hi commented Jan 27, 2024

im also affected by this...

@davidwinter
Copy link

Reading the comments, I can't see a fix that I can do for this, as I'm experiencing it too. Of course, other than to rollback to the previous version...

@escapedcat
Copy link
Member

@marcalexiei wdyt? Should we revert and and push a fix and add this again as breaking?

@marcalexiei
Copy link
Contributor Author


Warning

The source of this error is likely a mismatch of version between @commitlint packages in node_modules

Detailed explanation can be found in a comment above.

If someone is relying on a config which depends on an earlier version of @commitlint/config-conventional be sure to update it:

npm update @commitlint/config-conventional

I'm more than willing to find a fix for this issue.
I kindly ask to provide a minimal reproduction example by opening a new issue.

@davidwinter
Copy link

My project has Dependabot setup which was proposing a PR to just update @commitlint/config-conventional to 18.6.0. However, @commitlint/cli was still set to 18.4.4, which as mentioned, the mismatch in versions causes the issue.

Updating both at the same time, there is no issue.

Should both of the above packages set a minimum version number for each of the other packages if there is that hard dependency between them both? Or some other mitigation to prevent this?

@maninak
Copy link

maninak commented Jan 30, 2024

A CI job for my PR started suddenly failing with the error:

Run JulienKode/pull-request-name-linter-action@v0.5.0
Error: RangeError: Found invalid rule names: header-trim. Supported rule names are: body-case, body-empty, body-full-stop, body-leading-blank, body-max-length, body-max-line-length, body-min-length, footer-empty, footer-leading-blank, footer-max-length, footer-max-line-length, footer-min-length, header-case, header-full-stop, header-max-length, header-min-length, references-empty, scope-case, scope-empty, scope-enum, scope-max-length, scope-min-length, signed-off-by, subject-case, subject-empty, subject-full-stop, subject-max-length, subject-min-length, subject-exclamation-mark, trailer-exists, type-case, type-empty, type-enum, type-max-length, type-min-length
Error: Found invalid rule names: header-trim. Supported rule names are: body-case, body-empty, body-full-stop, body-leading-blank, body-max-length, body-max-line-length, body-min-length, footer-empty, footer-leading-blank, footer-max-length, footer-max-line-length, footer-min-length, header-case, header-full-stop, header-max-length, header-min-length, references-empty, scope-case, scope-empty, scope-enum, scope-max-length, scope-min-length, signed-off-by, subject-case, subject-empty, subject-full-stop, subject-max-length, subject-min-length, subject-exclamation-mark, trailer-exists, type-case, type-empty, type-enum, type-max-length, type-min-length

I hadn't changed my PR's title before and it's been passing for weeks. Then it started failing for no apparent reason. (I at first tried to shorten my PR title thinking that that was the issue but after reading the error better I realized there's more to it...)

here's my pr-linting.yaml

name: PR linting
on:
  pull_request:
    types: ['opened', 'edited', 'reopened', 'synchronize']

jobs:
  lint-pr-title:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v3
    - name: Install Dependencies
      run: npm install @commitlint/config-conventional
    - uses: JulienKode/pull-request-name-linter-action@v0.5.0

and my PR title is

feat(patch-detail): implement new view with in-depth patch info

@marcalexiei
Copy link
Contributor Author

marcalexiei commented Jan 30, 2024

  1. I kindly ask to provide a minimal reproduction example by opening a new issue.

  2. Could you please provide a minimal reproduction example in a separate issue?

For the third time, if you having issue open a separate issue providing a MINIMAL REPRODUCTION EXAMPLE


@escapedcat could you please lock the conversation so people might open separate issue instead of reporting them here?


@maninak for what I'm seeing the action you are referring to seems to use commitlint 17:

https://github.com/JulienKode/pull-request-name-linter-action/blob/900e046a8f2d6d0ba79bef8b6c731e4fe9f5fa8a/package.json#L48-L49

Please be sure to use consistent versions bettnwe @commitlint/{lint, rules,config} packages.
You are probably installing @commitlint/config-conventional@18.6.0 and running it with v17 packages.
Try installing a fixed version for @commitlint/config-conventional or update the action commitlint packages.

@escapedcat
Copy link
Member

A CI job for my PR started suddenly failing with the error:

This looks like an issue from https://github.com/JulienKode/pull-request-name-linter-action

@escapedcat could you please lock the conversation so people might open separate issue instead of reporting them here?

Let me have a look

@escapedcat
Copy link
Member

Please open a separate issue for further discussions, thanks

@conventional-changelog conventional-changelog locked as too heated and limited conversation to collaborators Jan 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants