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

jsdoc/check-line-alignment option to take into account existing alignment #732

Closed
TheJaredWilcurt opened this issue May 14, 2021 · 7 comments · Fixed by #740
Closed

jsdoc/check-line-alignment option to take into account existing alignment #732

TheJaredWilcurt opened this issue May 14, 2021 · 7 comments · Fixed by #740

Comments

@TheJaredWilcurt
Copy link

TheJaredWilcurt commented May 14, 2021

With my current settings, the linter wants to change my alignment from this

  /**
   * Description
   *
   * @param  {object} options  The user's options
   * @param  {string} message  The text to be logged
   * @param  {object} error    Optional object with additional details
   */

to this

  /**
   * Description
   *
   * @param {object} options The user's options
   * @param {string} message The text to be logged
   * @param {object} error   Optional object with additional details
   */

It should be able to detect that each section is starting at the same point on each line.

    'jsdoc/check-line-alignment': [
      1,
      'always',
      {
        'tags': [
          'param',
          'arg',
          'argument',
          'property',
          'prop',
          'returns',
          'return'
        ]
      }
    ],
@brettz9 brettz9 changed the title jsdoc/check-line-alignment does not take into account existing alignment jsdoc/check-line-alignment option to take into account existing alignment May 14, 2021
@brettz9
Copy link
Collaborator

brettz9 commented May 15, 2021

@renatho : Just pinging you (given your related work) in case you might have the inclination to implement.

@renatho
Copy link
Contributor

renatho commented May 15, 2021

Hey there!

I wonder about a case like this, for example:

  /**
   * Description
   *
   * @param {object}  options  The user's options
   * @param  {string} message  The text to be logged
   * @param  {object} error    Optional object with additional details
   */

What would be the correct line? Maybe we could add a new option that would be the size of the spacings. If we set it to 2, it would consider this correct:

  /**
   * Description
   *
   * @param  {object}  options  The user's options
   * @param  {string}  message  The text to be logged
   * @param  {object}  error    Optional object with additional details
   */

Would it work for you @TheJaredWilcurt?

@brettz9
Copy link
Collaborator

brettz9 commented May 15, 2021

Yeah, I went ahead and set the current tests to accept the lines at the beginning of the block as they are. Of course, if we implement the said feature, then indeed that sounds like a good solution to me (and also suggests the problematic line number if you were able to implement that also).

@TheJaredWilcurt
Copy link
Author

I guess I should explain my reasoning for the spaces.

Between @param and {object} is 2, but if there was an @return it would be 1. Because the { starts at the same place either way.

  /**
   * Description
   *
   * @param  {object} error  Text
   */
  /**
   * Description
   *
   * @param  {object} name  Text
   * @return {obect}         Text
   */

It's just easier to copy/paste common @params around if they always line up, whether there's a @return or not. It's not uncommon to refactor and break code up into smaller methods where sometimes it needs a return and sometimes it doesn't.

I never put more than one space between } and the variable name, because the curly brace syntax visually separates the information well already. Unless the types vary in length, but the longest still only gets one space.

  /**
   * Description
   *
   * @param  {object} error  Text
   */
  /**
   * Description
   *
   * @param  {object}  name  Text
   * @param  {boolean} bool  Text
   */

I always use two spaces between the variable name and description, because it's just plain text and can visually get blurred with each other.

This is why I was requesting the rule just check to see if each section starts at the same point in the line. But I was just thinking of it from a detection standpoint. I think your proposal was from an auto-fix standpoint. So I don't think it would be worth it to try to have some magic logic around the "2 spaces after tag if no return, or 1 if there is". As long as the linter can auto-fix the spacing after the tag, I'd be fine with it just being set to a number. But could we get greater granularity, like a option like this:

{
  // @param {string}
  afterTag: 1
  // {string} name
  afterVariableType: 1
  // name  Text
  afterVariableName: 2
}

@brettz9
Copy link
Collaborator

brettz9 commented May 16, 2021

Sure. And isn't it remarkable how much we all love our little jsdoc zen gardens enough to have these feelings of proportion and beauty about it? :-)

Thankfully with comment-parser's latest API, we can have such fine-grained control

As far as "2 spaces after tag if no return, or 1 if there is", this is actually not a problem for us because we already have a concept--not through comment-parser but through our use of code now extracted to @es-joy/jsdoccomment--of specifying which tags have names and which do not (including @param which does and @returns which does not).

I like your object proposal, though for simplicity (and consistency with comment-parser for that matter), I'd suggest the following (note, adding postDelimiter):

{
  postDelimiter: 1, // After the asterisk
  // @param {string}
  postTag: 1
  // {string} name
  postType: 1
  // name  Text
  postName: 2
}

@renatho
Copy link
Contributor

renatho commented May 16, 2021

Hey folks! I added a PR for that!
Please, let me know if it works for you. And I'd appreciate if you could help me testing if I didn't miss anything. =)

brettz9 added a commit that referenced this issue May 17, 2021
`customSpacings` option:

```js
        customSpacings: {
          postDelimiter: 2,
          postTag: 3,
          postType: 2,
          postName: 3,
        },
```

If the spacing is not defined, it uses 1 by default.

Co-authored-by: Renatho De Carli Rosa <[email protected]>
Co-authored-by: Brett Zamir <[email protected]>
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue May 17, 2021
brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue May 17, 2021
@gajus
Copy link
Owner

gajus commented May 17, 2021

🎉 This issue has been resolved in version 34.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label May 17, 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 a pull request may close this issue.

4 participants