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

Make recommended or a new config with logical rules #1175

Closed
JoshuaKGoldberg opened this issue Dec 29, 2023 · 7 comments · Fixed by #1298
Closed

Make recommended or a new config with logical rules #1175

JoshuaKGoldberg opened this issue Dec 29, 2023 · 7 comments · Fixed by #1298

Comments

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Dec 29, 2023

Motivation

I'd like to use the recommended ruleset from eslint-plugin-jsdoc only to enforce that JSDoc comments are correct in TypeScript repos. I don't want to enforce that they exist, just that if they do exist they're correct.

Current behavior

Right now, the steps are:

  1. Extend from plugin:jsdoc/recommended-typescript-error
  2. Manually disable jsdoc/require-* rules (jsdoc, param, returns, yields)

Desired behavior

My preference would be to have a breaking change to remove those rules from plugin:jsdoc/recommended-typescript and put them into a new preset config like stylistic-typescript. This would mirror how typescript-eslint has separate "stylistic" configs.

Failing that, it'd be nice to have a preset config with a name like plugin:jsdoc/logical.

Alternatives considered

Users like me can make our own shareable configs... but it'd be nice to not have to wrap around jsdoc's configs that way.

@brettz9
Copy link
Collaborator

brettz9 commented Dec 31, 2023

Despite the explanation on typescript-eslint.io I'm also unclear that these require-* rules could really be considered "stylistic". None of our rules really impact program logic. Maybe we could come up with some other dichotomous terminology?

@KilianU
Copy link

KilianU commented Jan 5, 2024

I think that the general issue here is the hierarchical configuration approach of eslint-plugin-jsdoc.
If I understand @JoshuaKGoldberg correctly, they basically wants to disable jsdoc/require-jsdoc while keeping all other rules. So they wouldn't get any warning/error if there is no docstring, but they would be notified if a parameter is missing.
A similar request was asked in #1169 where they don't want have all parameters documented, but if there is a documentation it should be correct.

@JoshuaKGoldberg using flat configs, you can already inherit a default configuration. However, the issue is that if you disable some rules, others will be disabled as well, because of the rule hierarchy.

const ruleSet = [
  // typescript specific rules - from https://github.com/typescript-eslint/typescript-eslint/issues/7694#issuecomment-1854655034
  ...compat.extends('plugin:@typescript-eslint/recommended'),
  ...compat.plugins('@typescript-eslint'),

  // enable eslint-plugin-jsdoc rules
  {
    // inherit eslint-plugin-jsdoc recommended configuration
    ...jsdoc.configs['flat/recommended-typescript'],
    // customisation:
    rules: {
      ...jsdoc.configs['flat/recommended-typescript'].rules
      'jsdoc/require-hyphen-before-param-description': 'error',  // wont trigger anything, because jsdoc/require-jsdoc is off :/
      'jsdoc/require-jsdoc': 'off',
    },
  },
]

@brettz9
Copy link
Collaborator

brettz9 commented Jan 5, 2024

I think that the general issue here is the hierarchical configuration approach of eslint-plugin-jsdoc. If I understand @JoshuaKGoldberg correctly, they basically wants to disable jsdoc/require-jsdoc while keeping all other rules.

However, the issue is that if you disable some rules, others will be disabled as well, because of the rule hierarchy.

<snip>
      'jsdoc/require-hyphen-before-param-description': 'error',  // wont trigger anything, because jsdoc/require-jsdoc is off :/
      'jsdoc/require-jsdoc': 'off',
<snip>

The rules are not actually hierarchical in this manner. If require-jsdoc is off, require-hypen-before-param-description will in fact trigger if there is a description present without a hyphen. Likewise can require-param, etc. be triggered.

So, what @JoshuaKGoldberg is requesting is simply a config to automatically disable the likes of:

  • require-jsdoc
  • require-param
  • require-returns
  • require-yields

I would presume the following would also be desired for disabling:

  • require-param-description
  • require-param-type
  • require-param-name
  • require-property
  • require-property-description
  • require-property-name
  • require-property-type
  • require-returns-description
  • require-returns-type

This will thus not require params, properties, return statements, etc., nor their names, types, or descriptions to be present, just that if present, they are valid (according to remaining rules such as check-param-names which insists that if there is something present like @param anArg, that anArg exists in the function).

There is a separate dimension here though, as to what is "stylistic". For example, jsdoc/no-multi-asterisks is not requiring anything, but it is also a stylistic question. The proposal should really be specific as to what is the expected grouping for each rule.

In my view, it could be more helpful for us to have a few more atomic groupings (possibly even overlapping), e.g., a require config, a stylistic config, a logically-valid config, etc.

@voxpelli
Copy link
Contributor

I do something similar in my config for types-in-js workflows, that skips the documentation aspect of jsdoc: https://github.com/voxpelli/eslint-config/blob/main/base-configs/jsdoc.js

  jsdoc.configs['flat/recommended-typescript-flavor'],
  {
    name: '@voxpelli/jsdoc',
    rules: {
      'jsdoc/check-types': 'off',
      'jsdoc/require-jsdoc': 'off',
      'jsdoc/require-param-description': 'off',
      'jsdoc/require-property-description': 'off',
      'jsdoc/require-returns-description': 'off',
      'jsdoc/require-yields': 'off',
      'jsdoc/tag-lines': ['warn', 'never', { 'startLines': 1 }],
      'jsdoc/valid-types': 'off',
    },
  },

Something like plugin:jsdoc/recommended-typescript-typed that does that would work well for me (also do note that its a long time since I set that up, so some options may be a bit needless nowadays)

@JoshuaKGoldberg
Copy link
Contributor Author

As of #1289, jsdoc/lines-before-block would also be disabled in that list.

recommended-typescript-typed

I like that naming!

If it helps, I'd be happy to send a PR with this, if given the 👍. 🙂

@brettz9
Copy link
Collaborator

brettz9 commented Aug 7, 2024

As of #1289, jsdoc/lines-before-block would also be disabled in that list.

Ok, so you're also talking about disabling truly stylistic items as well. But I think we should also have config which is for disabling requirements (e.g., require-param) but which includes the default style rules (some may like to keep things clean but not be so demanding as to the presence of docs). And maybe config which disables the style rules but which doesn't disable the require-param types (for those who want strict checking but don't care about the styling).

If it helps, I'd be happy to send a PR with this, if given the 👍. 🙂

Sure, go for it.

Thanks!

JoshuaKGoldberg added a commit to JoshuaKGoldberg/create-typescript-app that referenced this issue Aug 7, 2024
## Overview

General catch-all `ncu -u` and `pnpm dedupe`.

The latest versions of a few packages had required changes:
* `husky`: now has a smaller `.husky/*` hooks file format, yay!
* `eslint-plugin-jsdoc`: has a new stylistic rule added to recommended
(gajus/eslint-plugin-jsdoc#1175 (comment))
* `eslint-plugin-n`: noted that `parseArgs` was added as experimental in
`18.3.0`, so I bumped the minimum Node support version to `18.3.0`. That
was the _actual_ minimum version before; anything earlier would crash.

💖
Copy link

🎉 This issue has been resolved in version 50.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

4 participants