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

Empty docblocks are added above functions on lint #372

Closed
ay13 opened this issue Aug 27, 2019 · 15 comments · Fixed by #595
Closed

Empty docblocks are added above functions on lint #372

ay13 opened this issue Aug 27, 2019 · 15 comments · Fixed by #595

Comments

@ay13
Copy link

ay13 commented Aug 27, 2019

When I run the linter at adds empty docblocks above every function that doesn't already have a docblock. I assume this is part of the auto fixing but this messes doc coverage reports by incorrectly reporting that a function is documented when it is not.

Is there a way to disable the behavior of auto adding empty doc blocks?

@brettz9
Copy link
Collaborator

brettz9 commented Aug 29, 2019

ESLint unfortunately does not provide a means of selectively disabling rules for fixing; one apparently needs to specify a separate config for fixing (where the fixing config, in this case, would omit/disable the require-jsdoc rule).

@gajus and @golopot, what do you think of a disableFixer option that could be added to each rule to disable the fixer for any rule with a fixer? #336 already anticipates separate options for those rules that have multiple possible fixing strategies, so it might not be too big of a change.

It'd be nice if eslint offered an easier way, e.g., while loading a config, be able to disable specific rules within that config dynamically on the command line, but I received no feedback, at least on this comment on a related issue: eslint/eslint#11747 (comment) .

@gajus
Copy link
Owner

gajus commented Aug 30, 2019

I am probably misunderstanding something. If the intention is to have jsdoc block for every function, then require-jsdoc should be enabled and it is expected to "fix" codebase. If there is no such intention, then require-jsdoc should not be enabled and it is not anticipated to "fix" codebase.

To me this particular issue sounds like it is more about strictness of require-jsdoc. I would argue that a fixer that simply creates an empty jsdoc block doesn't add much value and just creates noise.

@brettz9
Copy link
Collaborator

brettz9 commented Aug 30, 2019

I am seeing the issue as the fact that projects will probably not want to go to the trouble of dynamically generating their config files (so that they could, for example, enable require-jsdoc in one non-fixing config to get linting warnings where jsdoc blocks were missing, but have the rule disabled in a separate --fix config so as not to get the comment block added), and would rather just use a single config file for both identifying linting issues and fixing them.

A related problem as I see it is that projects may differ in what they consider to be a reasonable fix. I would personally find the automatic addition of comment blocks to be useful, especially if starting with a project which had many items to be commented--it would also serve as a visual reminder of the need to document and an invitation to do so more quickly.

As discussed in #47, there may be other fixers which might be desired by some projects, but not others; for example, stripping bad param names might be seen as useful for removing stale param info by some projects, or a risk for removing mere bad spellings by another.

@carlrannaberg
Copy link

carlrannaberg commented Feb 27, 2020

This rule being as fixable is not very useful for us. Especially if it tries to fix it even when it is reported as warning.

Rule configuration:

'jsdoc/require-jsdoc': ['warn', {
	publicOnly: true,
	require: {
		ArrowFunctionExpression: true,
		ClassDeclaration: true,
		ClassExpression: true,
		FunctionDeclaration: true,
		FunctionExpression: true,
		MethodDefinition: true
	}
}]

This is what happens in our codebase when this rule is turned on:
Before:

module.exports = {
    set(key, value, ttl, callback) {
        // code
    }
};

After:

module.exports = {
    set/**
        *
        *//**
           *
           *//**
              *
              *//**
                 *
                 *//**
 *
 */
    (key, value, ttl, callback) {
        // code
    }
};

Having this kind of result unfortunately makes this rule pretty much useless for us. It would be great if the autofix could be turned off for this rule as I only want to see warnings about functions without JSDoc comments. Any idea how to accomplish that?

Currently the most viable option for just reporting the warnings without fixing is to use the deprecated ESLint rule require-jsdoc. Unfortunately it lacks some the configuration options that the jsdoc/require-jsdoc rule has.

@brettz9
Copy link
Collaborator

brettz9 commented Feb 28, 2020

@carlrannaberg : I think this might related to problems in the stringifier of comment-parser (or a limitation of what the stringifier could represent). I'm afraid I'm a bit tied up to investigate this. As a workaround, I can only suggest having a separate config, one which is for checking only (and has the rule enabled) and one for fixing (which has it disabled). But I'd suggest filing that as a separate issue as this particular issue is for an enhancement to disable fixing, but your case looks like a problem with the fixing itself.

@brettz9
Copy link
Collaborator

brettz9 commented Jun 28, 2020

Btw, @carlrannaberg, not sure if the issue was fixed by #524, but I am no longer seeing the recursive issue mentioned in #372 (comment)

@brettz9
Copy link
Collaborator

brettz9 commented Jun 28, 2020

@gajus , @golopot : I really hope we can at least keep the addition of empty jsdoc blocks as an option, as I find them very useful not only for saving a little time in data input but also for being a visual reminder of the need to document (and if one doesn't want empty descriptions to get past linting, one can use match-description to report on this). And getting documentation added is also useful because with rules being atomic, rules like require-returns, etc. won't report if the jsdoc block is missing, so allowing the auto-addition of doc blocks exposes a more complete list of required documentation to-dos.

However, as another request has been made to disable such a fixer (#594 ) I can make a PR to avoid the fixer, adding a enableFixer boolean (such as we have with the require-param and check-param-names rules) if that is all right. Let me know if you want a somewhat breaking change to make the option (to enable the fixer) off by default.

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Jun 28, 2020
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Jun 29, 2020
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Jun 29, 2020
brettz9 added a commit that referenced this issue Jun 30, 2020
@gajus
Copy link
Owner

gajus commented Jun 30, 2020

🎉 This issue has been resolved in version 28.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Jun 30, 2020
@brettz9
Copy link
Collaborator

brettz9 commented Jun 30, 2020

Note that the new enableFixer option is on by default currently, so to avoid the fixer, you have to explicitly set it to false.

@michaelfarrell76
Copy link

michaelfarrell76 commented Sep 3, 2020

hey @brettz9 any chance you have an example with the match-description setup? love that it generates the empty comment blocks but just trying to get the linter to report those empty comment blocks as errors. think i got it with valid-jsdoc but would love to figure out how to keep it with this rule.

particularly interested in getting this to work with type parameters

I've got something liek

interface PluginAPI {
    /** */
    readonly apiVersion: '1.0.0';
    /** */
    readonly command: string;
    /** */
    readonly viewport: ViewportAPI;
    closePlugin(message?: string): void;

and can't get the linter to report these empty comment block

using the contexts

{
      context: 'TSPropertySignature',
      inlineCommentBlock: true,
    },

@brettz9

This comment has been minimized.

@michaelfarrell76

This comment has been minimized.

@brettz9
Copy link
Collaborator

brettz9 commented Sep 3, 2020

Whoops, sorry! I forgot that we use jsdoc/require-description for the case of an entirely missing description. So you can instead use:

'jsdoc/require-description': ['error', {
    contexts: ['TSPropertySignature']
}]

@michaelfarrell76
Copy link

you are my life saver. appreciate your time!!

@brettz9
Copy link
Collaborator

brettz9 commented Sep 3, 2020

My pleasure! (Btw, I've hidden our comments, though they can still be seen, to avoid confusing people with the mistaken info I initially offered.)

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.

5 participants