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

feat: add require-asterisk-prefix rule #446

Merged
merged 11 commits into from May 10, 2021
Merged

Conversation

skeggse
Copy link
Contributor

@skeggse skeggse commented Dec 14, 2019

This rule checks that each line of the jsdoc comment starts with an asterisk. For composability with
other rules, this does not check the alignment or indentation of the comment content.

  • The naming of this rule is entirely provisional - better name suggestions welcome!

Fixes #199

@gajus
Copy link
Owner

gajus commented Dec 14, 2019

This rule should be require-asterisk-prefix with a toggle "always"/ "never". There are style guides that consider * prefix in comment body redundant.

@skeggse
Copy link
Contributor Author

skeggse commented Dec 14, 2019

The challenge I see with a "never" option is that I'm not sure there's an unambiguous way to lint that mode:

/**
 User encapsulates the data and methods needed to manage user objects.

 **Never instantiate this without a FIPS-compliant crypto module installed!**

 *some italics for good measure*

 @param {string} userId
 */
class User { ... }

Two lines in this jsdoc block in a naive solution would be incorrectly marked as problems due to having a *-prefix. Can you link me to the aforementioned style guides? A more formal definition of the syntax might help disambiguate these cases.

@skeggse skeggse changed the title feat: add check-prefix rule feat: add require-asterisk-prefix rule Dec 14, 2019
@leomeloxp
Copy link

The challenge I see with a "never" option is that I'm not sure there's an unambiguous way to lint that mode:

The way I interpret these options is:

"require-asterisk-prefix": [2, "always"]:

Always require a * as a prefix for JSDocs lines.

"require-asterisk-prefix": [2, "never"]:

Never require a * as a prefix for JSDocs lines. If they exist as something other than a prefix it's okay, and we can't really enforce a style guide for this, ie, you can have multiple adjacent lines beginning with a *.

The only check for never that I would possibly suggest would be to fail the check if ALL lines start with a * and follow the same pattern. Eg:

BAD

/**
 * A comment
 * that follows
 * a pattern
 */ 

GOOD

/**
 * A comment
 *that does
*not follows
 ** a pattern
 */ 

Thanks for putting this PR together. Hope my comment helps.

@skeggse
Copy link
Contributor Author

skeggse commented Dec 17, 2019

How does ["error", "never"] differ from "off", then? Just the check for all lines? Seems like a heuristic that'd be really rarely hit and be a false positive just as often as a true positive.

@leomeloxp
Copy link

If you add the check to ensure the * are not making a pattern that's where they'd differ (see GOOD and BAD examples above).

If you don't add that check then it would have no difference at all, which is not great but not necessarily a bad thing either in this case.

@brettz9
Copy link
Collaborator

brettz9 commented Dec 18, 2019

I figure some might also end up asking for an "asterisks multiline everywhere except in certain tags" style as well (e.g., @example or @license).

@skeggse skeggse marked this pull request as draft April 13, 2020 22:56
@brettz9
Copy link
Collaborator

brettz9 commented Jan 29, 2021

Both types should now be easier to support with comment-parser having been updated to allow for precise AST/tokens.

@brettz9 brettz9 force-pushed the fix-prefix branch 3 times, most recently from d50aace to 24fe60e Compare February 1, 2021 00:00
@brettz9 brettz9 force-pushed the fix-prefix branch 2 times, most recently from 917d748 to 4c97c2a Compare May 8, 2021 12:31
@brettz9 brettz9 marked this pull request as ready for review May 8, 2021 12:32
@brettz9
Copy link
Collaborator

brettz9 commented May 8, 2021

I think this PR may be ready now.

There can probably be improvements in adding missing asterisks, but it isn't always clear how to do this, e.g., if someone had:

    /**
     *
  Desc.
     */

or

    /**
     *
         Desc.
     */

...we should probably use the asterisks before and after to align them, but the problem is that any or all of them might be out of alignment, even on purpose. Right now, our fixer to add the asterisk just:

  1. If there was no text at all previously, set to the indent level
  2. Adds a space after the inserted asterisk if there is a tag or description (and sets it to nothing otherwise).

Eli Skeggs and others added 8 commits May 9, 2021 22:20
This rule checks that each line of the jsdoc comment starts with an asterisk. For composability with
other rules, this does not check the alignment or indentation of the comment content.

fix gajus#199
…pport; begin schema for options but options object not yet supported
@brettz9 brettz9 force-pushed the fix-prefix branch 2 times, most recently from fe5cfa9 to fd91593 Compare May 9, 2021 14:27
@brettz9
Copy link
Collaborator

brettz9 commented May 9, 2021

Two further comments:

  1. As mentioned we do little in this PR with asterisks being added in causing alignment questions or issues in the fixer. I think this should actually not be an issue, because there is already the check-alignment rule which can be used to clean that up further if desired.
  2. As far as the earlier discussion about what "never" means, since JSDoc does not appear to have specified that lines with tags must begin with an asterisk (they are not necessary in any case with /** @tag */), and since our JSDoc block parser, comment-parser, allows (reasonably I think) for tag lines without asterisks, "never" now truly means do not add them ever (though the tags option can be used to override where they should be required or optionally permitted (via the new "any" option)). If you want to enforce special alignment, that can be handled by check-alignment or a new option thereto (though I don't see a use case for requiring that asterisks be misaligned).

@brettz9 brettz9 merged commit f892338 into gajus:master May 10, 2021
@gajus
Copy link
Owner

gajus commented May 10, 2021

🎉 This PR is included in version 33.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: rule to enforce asterisks
4 participants