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

body-max-line-length should not report false positives for line starts with # #2964

Closed
4 tasks
xboy2012 opened this issue Jan 10, 2022 · 12 comments
Closed
4 tasks
Labels

Comments

@xboy2012
Copy link

@xboy2012 xboy2012 commented Jan 10, 2022

Expected Behavior

body-max-line-length should not report false positives for line starts with #

Current Behavior

body-max-line-length report false positives for line starts with #

While debuging in the node_modules/@commitlint/rules/lib/body-max-line-length.js

I found parsed.body have the following value as follows:

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# On branch chore/sync-remote-config2
# Your branch is up to date with 'origin/chore/sync-remote-config2'.
#
# Changes to be committed:
#       modified:   packages/code-standard-cli/package.json
#       new file:   packages/xxx/src/_private/yyyy/diffConfig.ts
#       renamed:   packages/xxx/src/_private/yyyy/foo/bar.ts ->  packages/xxx/src/_private/yyyy/foo/barabcdefg.ts
# 

Obviously, the lines come from the git commit command line, and should be ignored as they starts with #

Affected packages

  • cli
  • core
  • prompt
  • config-angular

Possible Solution

Steps to Reproduce (for bugs)

  1. First step
  2. Second step
commitlint.config.js ```js ```

Context

Your Environment

Executable Version
commitlint --version VERSION
git --version VERSION
node --version VERSION
@xboy2012
Copy link
Author

@xboy2012 xboy2012 commented Jan 10, 2022

It might be related to

https://github.com/conventional-changelog/commitlint/blob/master/%40commitlint/cli/src/cli.ts#L227

As I'm not using @commitlint/cli but @commitlint/load, there's no way to config parserOpts.commentChar = '#' in an easy way, I've add the lines by myself in the code base

@escapedcat
Copy link
Collaborator

@escapedcat escapedcat commented Jan 11, 2022

Thanks for reporting. Are you interested in creating a PR for this?

@escapedcat escapedcat added the bug label Jan 11, 2022
@Zirak
Copy link
Contributor

@Zirak Zirak commented Jan 13, 2022

When calling through @commitlint/load you can specify the parserOpts. For example:

const load = require('@commitlint/load').default
const lint = require('@commitlint/lint').default;

const config = {
    parserPreset: {
        parserOpts: {
            commentChar: '#', // <----
        },
    },
    rules: {
        'body-max-line-length': [2, 'always', 20]
    },
};

const opts = {
    parserOpts: config.parserPreset.parserOpts,
    plugins: {},
    ignores: [],
    defaultIgnores: true,
};

(async () => {
    const loaded = await load(config);
    
    const out = await lint(`feat: hello

# this is a very long line that exceeds the body-max-line-length rule, but has a comment character in the beginning so should be ignored
`, loaded.rules, opts);

    console.log(out.valid); // true
})();

Change the commentChar to something else to see it in effect.

Are you in a situation where that's difficult to pass in?

@escapedcat
Copy link
Collaborator

@escapedcat escapedcat commented Jan 13, 2022

Oh, thanks for you support here @Zirak !
Let me know if this is not a bug

@xboy2012
Copy link
Author

@xboy2012 xboy2012 commented Jan 13, 2022

When calling through @commitlint/load you can specify the parserOpts. For example:

const load = require('@commitlint/load').default
const lint = require('@commitlint/lint').default;

const config = {
    parserPreset: {
        parserOpts: {
            commentChar: '#', // <----
        },
    },
    rules: {
        'body-max-line-length': [2, 'always', 20]
    },
};

const opts = {
    parserOpts: config.parserPreset.parserOpts,
    plugins: {},
    ignores: [],
    defaultIgnores: true,
};

(async () => {
    const loaded = await load(config);
    
    const out = await lint(`feat: hello

# this is a very long line that exceeds the body-max-line-length rule, but has a comment character in the beginning so should be ignored
`, loaded.rules, opts);

    console.log(out.valid); // true
})();

Change the commentChar to something else to see it in effect.

Are you in a situation where that's difficult to pass in?

Thanks for the description, I passed in commentChar: '#' in our team's private changelog-preset, eg: @xxx/conventional-changelog

Then in the .commitlintrc.js I wrote the following codes:

module.exports = {
    extends: ['@commitlint/config-conventional'],
    parserPreset: require.resolve('@xxx/conventional-changelog'),
  }

But @commitlint/load doesn't handle all edge cases of a third-party changelog-preset.

To be brief, several format of exports are valid:

  1. object
  2. Promise of an object
  3. function that return an object
  4. function that return a Promise of object.

Unfortunately, @commitlint/load just require('@xxx/conventional-changelog') which only match case 1

Then I have to manually handle 2-4 logic.

Would be appreciated if all edge case could be handled and the type should not be just unknown.

export interface ParserPreset {
    name?: string;
    path?: string;
    parserOpts?: unknown;
}

@Zirak
Copy link
Contributor

@Zirak Zirak commented Jan 13, 2022

Ah I see, so it's a broken expectation between how a rule is resolved and how parserPreset is resolved.

It seems like load-parser-opts takes care of cases 1 and 2 that you mentioned, allowing you to export an object or a promise of an object.

Correct me if I'm wrong here @escapedcat (or someone else), it seems like this is a feature, and 3 and 4 could be supported by changing this condition:

// Create parser opts from factory
if (
isParserOptsFunction(parser) &&
typeof parser.name === 'string' &&
parser.name.startsWith('conventional-changelog-')
) {

...to not only check for functions whose name begins with conventional-changelog-. i.e. remove line 48. Thoughts?

@Zirak
Copy link
Contributor

@Zirak Zirak commented Jan 13, 2022

Actually my bad, I read that piece of code completely wrong. We won't even reach that part because of an earlier check up in the function:

if (!pendingParser || typeof pendingParser !== 'object') {
return undefined;
}

So inside this block (or elsewhere if it fits the flow), something like the following:

if (typeof parser === 'function') {
  return loadParserOpts(parser());
}

@xboy2012
Copy link
Author

@xboy2012 xboy2012 commented Jan 14, 2022

typeof parser.name === 'string' &&

I just wonder why these two lines exists?

    // Create parser opts from factory
    if (isParserOptsFunction(parser) &&
        typeof parser.name === 'string' &&    // why this line ? 
        parser.name.startsWith('conventional-changelog-')) { // why this line ? 

Why the following codes are not OK?

 // Create parser opts from factory
    if (isParserOptsFunction(parser)) {

@escapedcat
Copy link
Collaborator

@escapedcat escapedcat commented Jan 14, 2022

@armano2 you have an idea or input to this? Sorry for bothering!

@Zirak
Copy link
Contributor

@Zirak Zirak commented Jan 15, 2022

Why the following codes are not OK?

// Create parser opts from factory
if (isParserOptsFunction(parser)) {

From my understanding @xboy2012, that logic isn't reached. Since earlier in the function there's a check which filters out all non-objects, no matter what function is passed in the result is undefined:

loadParserOpts(() => {}) // undefined

It applies to passing in an object with a parserOpts function and a certain name, like:

loadParserOpts({
  name: 'conventional-changelog-example',
  parserOpts: () => {}, // <-- this will be called
})

As I mentioned above, function support could be added by conditionally checking for passing in functions earlier in the code, and dealing with that there. If there's interest I can draw up a PR.

Zirak added a commit to Zirak/commitlint that referenced this issue Jan 19, 2022
Previously, `load` could accept `parserPresets` as both values and promises of
values. Per conventional-changelog#2964 however, there was expectations for it to also support
functions returning said values.

This commit enables to specify `parserPresets` in all the same ways as
rules. That is, both commitlint rules and `parserPresets` can be specified as:

- Plain values
- Promises
- Functions returning plain values
- Functions returning promises

Solves conventional-changelog#2964.
Zirak added a commit to Zirak/commitlint that referenced this issue Jan 19, 2022
Previously, `load` could accept `parserPresets` as both values and promises of
values. Per conventional-changelog#2964 however, there was expectations for it to also support
functions returning said values.

This commit enables to specify `parserPresets` in all the same ways as
rules. That is, both commitlint rules and `parserPresets` can be specified as:

- Plain values
- Promises
- Functions returning plain values
- Functions returning promises

Solves conventional-changelog#2964.
escapedcat pushed a commit that referenced this issue Jan 20, 2022
Previously, `load` could accept `parserPresets` as both values and promises of
values. Per #2964 however, there was expectations for it to also support
functions returning said values.

This commit enables to specify `parserPresets` in all the same ways as
rules. That is, both commitlint rules and `parserPresets` can be specified as:

- Plain values
- Promises
- Functions returning plain values
- Functions returning promises

Solves #2964.
@escapedcat
Copy link
Collaborator

@escapedcat escapedcat commented Jan 20, 2022

Should be fixed with 16.1.0.
@xboy2012 please give it a try and close this this issue if it works. Thanks.
@Zirak thanks for your PR!

@escapedcat
Copy link
Collaborator

@escapedcat escapedcat commented Feb 13, 2022

Closing this because of no negative feedback so far

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

No branches or pull requests

3 participants