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

Warn on empty trailing line in JSDoc comment #847

Closed
Zamiell opened this issue Mar 7, 2022 · 18 comments · Fixed by #848
Closed

Warn on empty trailing line in JSDoc comment #847

Zamiell opened this issue Mar 7, 2022 · 18 comments · Fixed by #848

Comments

@Zamiell
Copy link
Contributor

Zamiell commented Mar 7, 2022

Hello and thanks for the fantastic linter.

Consider the following code snippet:

/**
 * This function does lots of things.
 *
 */
function foo() {}

The final line in the comment is superfluous. Thus, the code can be simplified to the following:

/**
 * This function does lots of things.
 */
function foo() {}

In fact, I expected eslint-plugin-jsdoc to automatically warn/fix this for me, since it does other helpful transformations of this nature.

Is there a particular reason why the plugin doesn't currently do this transformation? If not, can this be something added?

@Zamiell
Copy link
Contributor Author

Zamiell commented Mar 11, 2022

Thank you brettz!!

Follow-up question: Is this new rule part of the recommended config? I think having it in there is a good idea.

@gajus
Copy link
Owner

gajus commented Mar 11, 2022

🎉 This issue has been resolved in version 38.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Mar 11, 2022
@brettz9
Copy link
Collaborator

brettz9 commented Mar 11, 2022

Let's first confirm that the rules are working as desired, and then we can see about switching the default in the future. It was already a breaking change.

Please note that you have to configure two rules to get the desired behavior. This is because it makes a difference whether there is a tag followed by a newline or there are no tags.

In the case of removing the trailing line when there are no tags, you need to enable match-description (which is not a recommend rule) with the object option set to something like matchDescription: '[\\s\\S]*\\S$'. This is a permissive expression which should not insist on anything besides that the final value be non-whitespace. It does not have a fixer, however.

For the case of tags followed by an extra newline, you need to set dropEndLines: true on the tag-lines rule. Note that you can use this option even if you set the first string option to any.

@Zamiell
Copy link
Contributor Author

Zamiell commented Mar 11, 2022

Ok, I'm trying it out, and I have the following in my ESLint config:

    "jsdoc/match-description": [
      "warn",
      {
        matchDescription: "[\\s\\S]*\\S$",
      },
    ],

Yet ESlint still fails to show any problems with the following code:

/**
 * This function does lots of things.
 *
 */

I do appear to be using the latest version of the plugin:

$ grep version node_modules/eslint-plugin-jsdoc/package.json
  "version": "38.0.0"

Any ideas?

@Zamiell
Copy link
Contributor Author

Zamiell commented Mar 11, 2022

I'll also quickly record here, before I forget, that it would be nice for superfluous leading lines to be automatically removed as well:

/**
 *
 * This function does lots of things.
 */

@Zamiell
Copy link
Contributor Author

Zamiell commented Mar 11, 2022

For the case of tags followed by an extra newline, you need to set dropEndLines: true on the tag-lines rule.

This appears to be false, as I don't seem to have to specify anything.
In other words, when using the recommended config on the latest version, Prettier will automatically convert:

  /**
   * @param foo
   * @param bar
   *
   */

to this:

  /**
   * @param foo
   * @param bar
   */

@brettz9
Copy link
Collaborator

brettz9 commented Mar 11, 2022

Ok, I'm trying it out, and I have the following in my ESLint config:

    "jsdoc/match-description": [
      "warn",
      {
        matchDescription: "[\\s\\S]*\\S$",
      },
    ],

Yet ESlint still fails to show any problems with the following code:

/**
 * This function does lots of things.
 *
 */

I do appear to be using the latest version of the plugin:

$ grep version node_modules/eslint-plugin-jsdoc/package.json
  "version": "38.0.0"

Any ideas?

Unfortunately, I'm not able to replicate. You might try a fresh install of node_modules. Otherwise, will need a minimal test reproduction repo as .

@brettz9
Copy link
Collaborator

brettz9 commented Mar 11, 2022

I'll also quickly record here, before I forget, that it would be nice for superfluous leading lines to be automatically removed as well:

/**
 *
 * This function does lots of things.
 */

You can use the following expression:

 "jsdoc/match-description": [
      "warn",
      {
        matchDescription: "^\\S[\\s\\S]*\\S$",
      },
    ],

@brettz9
Copy link
Collaborator

brettz9 commented Mar 11, 2022

For the case of tags followed by an extra newline, you need to set dropEndLines: true on the tag-lines rule.

This appears to be false, as I don't seem to have to specify anything. In other words, when using the recommended config on the latest version, Prettier will automatically convert:

  /**
   * @param foo
   * @param bar
   *
   */

to this:

  /**
   * @param foo
   * @param bar
   */

Did you update Prettier? Maybe they started stripping this or something, but in any case, that is a separate tool unrelated to this project, so while I'm glad it works for you, indeed for other users at least, it should still be necessary to add the option I mentioned.

@Zamiell
Copy link
Contributor Author

Zamiell commented Mar 11, 2022

I tried in a brand new repo with only eslint-pluign-jsdoc installed and nothing else, and it still doesn't work, so I must insist that it does seem to be bugged: https://github.com/Zamiell/eslint-plugin-jsdoc-bug

@brettz9
Copy link
Collaborator

brettz9 commented Mar 11, 2022

The match-description rule only checks JSDoc blocks attached to functions by default. Setting contexts: ['any'] causes it to work in your example (as does switching the logging statement to a function).

@brettz9
Copy link
Collaborator

brettz9 commented Mar 12, 2022

Regarding dropEndLines, note that the recommended default includes the tag-lines rule. Since the default of this rule is "never", it will indeed complain about empty lines following @param, so I guess this is what you were seeing.

However, if you don't want to enforce this rule reporting empty lines after tags (even lines after tags which are followed up by another tag), in that case, you will need dropEndLines: true (with the first argument set to "any").

@Zamiell
Copy link
Contributor Author

Zamiell commented Mar 15, 2022

Another question:

    "jsdoc/match-description": [
      "warn",
      {
        matchDescription: "^\\S[\\s\\S]*\\S$",
        contexts: ["any"],
      },
    ],

This regex causes the following code to fail:

  /** Does something very important. */
  function foo(): string;

Is there a way to tweak the regex such that it doesn't care about JSDoc comments that are all on one line? In this circumstance, I don't want the linter to complain, because there is not any superfluous leading or training newlines.

@brettz9
Copy link
Collaborator

brettz9 commented Mar 15, 2022

While, purely technically speaking, at least from our perspective, that's not superfluous whitespace in the end in the sense that unlike the initial space in the block, JSDoc does allow the trailing whitespace to be omitted, indeed it is unlikely that most people will want to report that trailing whitespace in a single line block.

You can avoid single line blocks by the following:

          contexts: [
            {
              comment: 'JsdocBlock[endLine!=0]',
            },

This will check every JSDoc comment except for single line ones.

Note that this comment AST is experimental and may change format in the future.

@Zamiell
Copy link
Contributor Author

Zamiell commented Mar 15, 2022

Thanks. We are making progress, but I have yet another question.

Using this rule:

    // Prevent superfluous leading and trailing newlines
    "jsdoc/match-description": [
      "warn",
      {
        matchDescription: "^\\S[\\s\\S]*\\S$",
        contexts: [
          {
            comment: "JsdocBlock[endLine!=0]",
          },
        ],
      },
    ],

It throws a warning for the following code:

  /**
   * This is my favorite function, foo.
   *
   * @returns Nothing.
   */
  function foo(): void;

But this is not desirable, because there aren't any superfluous leading or trailing newlines.
Any ideas?

@brettz9
Copy link
Collaborator

brettz9 commented Mar 15, 2022

Before replying to your last comment, let me say that if you wanted to also be able to lint 0-length comments, although match-description can't have different main description regexes for different contexts, you could use no-restricted-syntax with something like this:

`jsdoc/no-restricted-syntax: ['error', {
  contexts: [
    {
      comment: 'JsdocBlock[endLine=0][description!=/^\\S[\\s\\S]*\\S\\s$/]',
    }
  ]
}]

You could also add your context for multi-line ones in the same array (and just avoid using match-description in favor of this rule).

@brettz9
Copy link
Collaborator

brettz9 commented Mar 16, 2022

With match-description, you can use JsdocBlock[endLine!=0]:not(:has(JsdocTag)).

Note that I noticed that no-restricted-syntax does not get the pre-tag newline added in the description, so you cannot assume the line break is even there at the end when checking [description=/.../] even though it should be (match-description is using more than the AST to determine that there is an extra newline). Hopefully that can be fixed in the future, but it's not on my agenda for now (it's not trivial because I believe comment-parser is not giving us that info directly so we'd have to detect it).

@Zamiell
Copy link
Contributor Author

Zamiell commented Mar 16, 2022

That worked! Thanks again.

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Mar 18, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint-plugin-jsdoc](https://github.com/gajus/eslint-plugin-jsdoc) | devDependencies | major | [`37.9.7` -> `38.0.4`](https://renovatebot.com/diffs/npm/eslint-plugin-jsdoc/37.9.7/38.0.4) |

---

### Release Notes

<details>
<summary>gajus/eslint-plugin-jsdoc</summary>

### [`v38.0.4`](https://github.com/gajus/eslint-plugin-jsdoc/releases/v38.0.4)

[Compare Source](gajus/eslint-plugin-jsdoc@v38.0.3...v38.0.4)

##### Bug Fixes

-   **`require-jsdoc`:** allow `TSTypeLiteral` and `TSTypeAliasDeclaration` to have `TSPropertySignature` checks pass through them toward public export for `publicOnly` checks; fixes [#&#8203;852](gajus/eslint-plugin-jsdoc#852) ([19e4f6f](gajus/eslint-plugin-jsdoc@19e4f6f))

### [`v38.0.3`](https://github.com/gajus/eslint-plugin-jsdoc/releases/v38.0.3)

[Compare Source](gajus/eslint-plugin-jsdoc@v38.0.2...v38.0.3)

##### Bug Fixes

-   **`valid-types`:** update `es-joy/jsdoccomment` ([5e8e0c7](gajus/eslint-plugin-jsdoc@5e8e0c7))

### [`v38.0.2`](https://github.com/gajus/eslint-plugin-jsdoc/releases/v38.0.2)

[Compare Source](gajus/eslint-plugin-jsdoc@v38.0.1...v38.0.2)

##### Bug Fixes

-   **`match-description`:** single empty line was not being reported ([ec34e66](gajus/eslint-plugin-jsdoc@ec34e66))

### [`v38.0.1`](https://github.com/gajus/eslint-plugin-jsdoc/releases/v38.0.1)

[Compare Source](gajus/eslint-plugin-jsdoc@v38.0.0...v38.0.1)

##### Bug Fixes

-   **`match-name`:** perform replacements for names appearing after multiline types ([a23168d](gajus/eslint-plugin-jsdoc@a23168d))

### [`v38.0.0`](https://github.com/gajus/eslint-plugin-jsdoc/releases/v38.0.0)

[Compare Source](gajus/eslint-plugin-jsdoc@v37.9.7...v38.0.0)

##### Bug Fixes

-   **`match-description`:** adjust default to allow for trailing whitespace but do check for such WS now ([a31a8fd](gajus/eslint-plugin-jsdoc@a31a8fd))

##### Features

-   **`tag-lines`:** add `dropEndLines` option; fixes [#&#8203;847](gajus/eslint-plugin-jsdoc#847) ([26c1c2c](gajus/eslint-plugin-jsdoc@26c1c2c))

##### BREAKING CHANGES

-   **`match-description`:** `match-description` regular expressions now need to take account for trailing whitespace

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1216
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants