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

[match-description] Pattern is not correctly applied (newlines not preserved) #692

Closed
Shinigami92 opened this issue Feb 15, 2021 · 10 comments

Comments

@Shinigami92
Copy link

Expected behavior

No warnings should be printed / detected

I have setup following pattern: "^([A-Z`].*(\\.|:)|-\\s.*)$"

  • Should start with a capital letter or backtick [A-Z`]
  • Followed by anything .*
  • Should end with a period or colon (\\.|:)
  • Or the whole line can be a bullet point (list entry) -\\s.*

Actual behavior

71:0 warning JSDoc description does not satisfy the regex pattern jsdoc/match-description

ESLint Config

    //...
    "jsdoc/match-description": [
      "warn",
      {
        "mainDescription": "^[A-Z`].*\\.$",
        "matchDescription": "^([A-Z`].*(\\.|:)|-\\s.*)$", // This is the pattern I want to apply
        "contexts": ["any"],
        "tags": {
          "param": true,
          "returns": true
        }
      }
    ]
    //...

https://github.com/mib200/vue-gtm/blob/fa0b139417b09c871ae27c629415dea06ec50a49/.eslintrc.json#L49-L60

  /**
   * Enable or disable plugin.
   *
   * When enabling with this function, the script will be attached to the `document` if:
   *
   * - the script runs in browser context
   * - the `document` doesn't have the script already attached
   * - the `loadScript` option is set to `true`
   *
   * @param enabled `true` to enable, `false` to disable. Default: `true`.
   */

https://github.com/mib200/vue-gtm/blob/fa0b139417b09c871ae27c629415dea06ec50a49/src/plugin.ts#L70-L80

Environment

  • Node version: v15.6.0
  • ESLint version v7.20.0
  • eslint-plugin-jsdoc version: 32.0.0

If I do anything wrong on my side, please explain how I can fix it 🙂

@Shinigami92
Copy link
Author

It looks like my mainDescription was wrong. I switched to ^[A-Z`].+?[\\.:](.*(- .+|_.*_))?$ but now it fails to validate on:

  /**
   * Tasks map
   *
   * - `key`: The `Worker`
   * - `value`: Number of tasks that has been assigned to that worker since it started
   */

I want to enforce that Tasks map ends with a period.

Can you give me a hint how to fix this?

@ST-DDT
Copy link

ST-DDT commented Feb 15, 2021

The first part of your pattern ([A-Z`].+?[\\.:]) matches everything up to - `key`:.
Then (.* matches The `Worker`
And finally (- .+|_.*_) matches - `value`:....

If you add some debug output somewhere there you will notice, that the parsed description does not contain any newlines, with makes it virtually impossible to prevent the patterns from matching in this scenario.

AFAICT the only way to solve this, is to not strip the newlines inside this plugin. However this might break existing patterns, so to avoid breaking existing patterns, the regex should be created with the us flag which allows . to match the then preserved \n.

@brettz9
Copy link
Collaborator

brettz9 commented Feb 16, 2021

There is not a sophisticated parser for this rule, so currently you would either need punctuation at the end of

option is set to true

... or you would need to change your regex to allow for this.

So yes, @ST-DDT 's observation is quite correct, and the suggestion apt. I just pushed a fix/release so that one can supply slash-delimited regexes with flags to match-description as with some other rules (including the s flag). Note that for future reference, if you want something like . which matches newlines when you don't have the s flag, you can use something like [\s\S].

I'm still working on a fix so that newlines are preserved in match-description (except for a single trailing one). The change to support newlines is required in a number of places, including in a few other rules which I had not discovered the new behavior in the block and tag description's upon the comment-parser update. We already have an internal utility (utils.getDescription) for getting at the block descriptions, but needs some implementing/reviewing/testing. Have started some work on it, but not sure when I may be able to finish it up.

@brettz9 brettz9 changed the title [match-description] Pattern is not correctly applied [match-description] Pattern is not correctly applied (newlines not preserved) Feb 16, 2021
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Feb 16, 2021
…or purposes of regex matching; fixes part of gajus#692

Should now include fix for `comment-parser` update's dropping of newlines in jsdoc block `description`; still need to preserve newlines across rules in the `.description` of tags
@brettz9
Copy link
Collaborator

brettz9 commented Feb 16, 2021

I've pushed a fix/release now which should address the immediate concern of this issue.

However, I'll keep the issue open as the following rules need to be investigated for their lack of preservation of newlines within checks of individual tag description's (like @description or @example). (Some other rules use this property also, but should not be of concern since they are only checking for non-whitespace.)

  • check-examples
  • check-values
  • match-description
  • require-description
  • require-description-complete-sentence
  • require-example
  • require-hyphen-before-param-description
  • valid-types

@Shinigami92
Copy link
Author

I applied 74bbe38 locally and it's working 😃
I can check against patterns that includes \\n 🎉

When I have setup my complete pattern, I will send it here, so you can see what edge cases I have solved with it.

@Shinigami92
Copy link
Author

So my current patterns are these:

    "jsdoc/match-description": [
      "warn",
      {
        "mainDescription": "/^[A-Z`].+?(\\.|:)(\\n\\n.*((\\n{1,2}- .+)|(_.+_)|`.+`|\\n\\n---))?$/us",
        "matchDescription": "^[A-Z`].+(\\.|`.+`)$",
        "contexts": ["any"],
        "tags": {
          "param": true,
          "returns": true
        }
      }
    ]

I only have a wired problem with jsdocs like

/** Wrap attributes threshold. */
export const WRAP_ATTRIBUTES_THRESHOLD: IntSupportOption = { // This line shows warning: "eslintjsdoc/match-description"
	since: '1.8.0',
	category: CATEGORY_PUG,
	type: 'int',
	default: -1,
	description: 'The maximum amount of attributes that an element can appear with on one line before it gets wrapped.',
	range: { start: -1, end: Infinity, step: 1 }
};

https://github.com/prettier/plugin-pug/blob/56e1fe00c9f657652ffa02c65f982eb8f423a883/src/options/wrap-attributes.ts#L4-L5

But this could be an issue on my side.
And what makes it even more wired is, that it only is a problem in this one project and my other two OpenSource projects doesn't have this problem 🤷

IMO you could release the new version because it is a HUGE improvement 🎉

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Feb 16, 2021
…trailing whitespace is stripped for comparisons; for gajus#692
@brettz9
Copy link
Collaborator

brettz9 commented Feb 16, 2021

Released in 32.0.2 along with a fix for your last example.

(FWIW, after preserving whitespace, the reason your last example was failing was because I was only stripping trailing newlines, but for single-line examples at least, it becomes necessary to allow for trailing spaces since the final space in /** text */ gets treated as part of the description when the parser preserves all whitespace as we need.)

Still not handled for tags, however.

This was referenced Mar 11, 2021
This was referenced Mar 16, 2021
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Apr 28, 2021
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Apr 28, 2021
@brettz9
Copy link
Collaborator

brettz9 commented Apr 28, 2021

If you all could test out #718 , it'd be nice to ensure the fixes are working. Should now be applied to all relevant rules.

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Apr 28, 2021
@gajus
Copy link
Owner

gajus commented Apr 30, 2021

🎉 This issue has been resolved in version 32.3.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Apr 30, 2021
@Shinigami92
Copy link
Author

@brettz9 Until now I didn't encountered any issue in my projects 🙂

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

No branches or pull requests

4 participants