Add line alignment tags support when using always

[renatho]

Part of #680
Close #689

This PR adds support to the tags option when using the always option.

The first PR for this feature was the #689, but the approach wasn't being enough, needing a lot of hacks. So I changed the approach, creating a new transform based on the original align, but adding support to custom tags.

It still reports the errors for the entire block, different than what was suggested in #680 (comment).

Another idea for a new feature in the future:

• We could add a new option to force the alignment similar to the never for the tags that aren't in the tags when using the always.

[renatho]

Ah, it still needs some more tests for full coverage.

EDIT: Done! ✅

[TheJaredWilcurt]

Ran it against my codebase and it worked without any bugs. I just wish that you could have 2 spaces after a variable name. without it, it becomes hard to read and looks like it's all just one sentence. example:

/**
 * Mocks the process.platform to a specified value.
 *
 * @param {string} platform 'win32', 'linux', or 'darwin'.
 */

I'd prefer a two space buffer before and after a variable name

/**
 * Mocks the process.platform to a specified value.
 *
 * @param {string}  platform  'win32', 'linux', or 'darwin'.
 */

would be nice if there was a setting for this like min-spaces-before-variable-name: 2 and min-spaces-before-variable-description: 2

[renatho]

@TheJaredWilcurt, thank you for testing! About these other requisites, I think it could be part of a different PR.

I also tested it against the Gutenberg project and I identified only one issue. Basically, if we have only the @return tag, it'll add 2 spaces after the tag. I think it's because the approach used with the tokenizers. This issue is currently present already, so I think we could merge this that already have improvements, and we could fix it in another PR.

I'm not sure yet how we could fix this. Do you have any idea @brettz9? I'd like to fix it without changing the align transformer because I tried to make it more similar to the original as possible, so we could remove this if we add it to the comment parser lib. If you don't have any idea, maybe I'll try to fix it in the align transformer code.

[renatho]

I checked a little more, and actually I think there is not problem with how it's using the tokenizers. So I already fixed it through the transformer in this commit: 6d29610

Anyway, I'd appreciate your feedback when you have some time, @brettz9 =)

[renatho]

I found another issue related to the indentation with tabs. Fixed here: 8389ad3

[renatho]

It's almost there. Now I found another weird error with the template tag (duplicating some content).

I think it's probably because this, but I'll try to check it another day.

[renatho]

It's almost there. Now I found another weird error with the template tag (duplicating some content).

I think it's probably because this, but I'll try to check it another day.

I took some time to debug it. The issue is that we're getting the name and the description tokens duplicated in the following case, for example:

          /**
           * With template tag.
           *
           * @template T
           *
           * @param {string} lorem Description.
           * @param {int}    sit   Description multi words.
           */
          const fn = ( lorem, sit ) => {}

See the result debugging the commented part.

The same happens for @template {string} T.

[renatho]

Hey @brettz9!
Do you have any idea of where the error could be in that part I mentioned in the previous comment? I suspect it could be in the pos, but honestly, I'm a little confused about the meanings of the variables remainder, pos, and extra. If you have some clues, I could try to fix it.

[brettz9]

If you log against the existing tests, you can see spec.source[0].tokens, and how description gives the remainder after @template (there's no need for name as it is not populated).

pos is the position at which the first whitespace that is itself not preceded by whitespace or a comma is found (i.e., where the template name or sequence of names stop).

extra is then everything after that post-name(s) whitespace (i.e., any extra postName whitespace and any description).

[renatho]

If you log against the existing tests, you can see spec.source[0].tokens, and how description gives the remainder after @template (there's no need for name as it is not populated).

pos is the position at which the first whitespace that is itself not preceded by whitespace or a comma is found (i.e., where the template name or sequence of names stop).

extra is then everything after that post-name(s) whitespace (i.e., any extra postName whitespace and any description).

Hey @brettz9 !

Thank you for your explanation! I was creating a fix (just adding a conditional to the postName and description):

        let postName = '';
        let description = '';
        if (pos !== -1) {
          [, postName, description] = extra.match(/(\s*)(.*)/);
        }

But I noticed that part was extracted for another dependency (@es-joy/jsdoccomment), right? I checked the dependency, but the repo URL didn't open for me. I'm thinking that we could merge this PR, and introduce this fix to @es-joy/jsdoccomment. Do you think you could do that?

[brettz9]

The repo is at https://github.com/es-joy/jsdoccomment , but I can add if I can see the reason for it. However, I don't imagine we can add such a conditional because if it is something like the example used at https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#template * @template {string} K - K must be a string or string literal, making it conditional will shave off the postName and description. What is the concrete problem you are trying to solve in making this change?

[renatho]

The repo is at https://github.com/es-joy/jsdoccomment , but I can add if I can see the reason for it. However, I don't imagine we can add such a conditional because if it is something like the example used at https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#template * @template {string} K - K must be a string or string literal, making it conditional will shave off the postName and description. What is the concrete problem you are trying to solve in making this change?

Hey @brettz9 !

I'm not in the computer now, but I remember I tested with the examples of this link and it worked well.

When pos is not found (-1), I think it means we don't have anything after the name, right?

Basically, the problem is with a case like (when we don't have anything after the name):

@template T

In this case, it's setting T for name and for description.

[brettz9]

Ah yes, sorry, @renatho . Fixed in 31.1.1

[renatho]

Nice!!! Than you @brettz9 !! \o/
In this case, this PR is ready for a final review :)

Update: Just added a test with the @template tag using multiple formats.

[brettz9]

Thanks for all your work and persistence on this, @renatho !

[renatho]

Thanks for all your work and persistence on this, @renatho !

Thank you for all your support @brettz9! =)
Just to know, is there a plan to release these changes soon?

[brettz9]

Ugh, seems my commit message was not well-formed. Adding a commit to revert and yet another to add back in the proper format.

[gajus]

🎉 This PR is included in version 33.3.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀