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

Add lines alignment rule #636

Merged
merged 12 commits into from
Sep 14, 2020
Merged

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Sep 11, 2020

The PR

When developing for WordPress, there is a recommended standard to have the comment lines aligned.

Some days ago, we created an Eslint rule for that: https://github.com/Automattic/eslint-plugin-jsdoc-alignment/
It was created with the purpose to start using it and soon submit a contribution to add it to the eslint-plugin-jsdoc, which makes much more sense.

If this PR is approved, the current eslint-plugin-jsdoc-alignment will be deprecated in favor of this rule.

It's my first contribution to eslint-plugin-jsdoc, so I'm sorry if I missed something. But I'll be happy to fix anything if needed.

Example

❌ Incorrect example:

/**
 * Function description.
 *
 * @param {string} lorem Description.
 * @param {int} sit Description multi words.
 */
const fn = ( lorem, sit ) => {}

✅ Correct example:

/**
 * Function description.
 *
 * @param {string} lorem Description.
 * @param {int}    sit   Description multi words.
 */
const fn = ( lorem, sit ) => {}

Copy link
Collaborator

@brettz9 brettz9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides my comments, this looks good to me from an initial look.

It would be ideal if we could in the future instead use comment-parser for obtaining the tag, name, and description with whitespace, but comment-parser doesn't currently support that, so regexes should work for now.

If you could make the changes per the comments, I can see about taking a closer look locally. Thanks a lot!

src/rules/checkLinesAlignment.js Outdated Show resolved Hide resolved
src/rules/checkLinesAlignment.js Show resolved Hide resolved
.README/rules/check-lines-alignment.md Show resolved Hide resolved
@renatho
Copy link
Contributor Author

renatho commented Sep 12, 2020

@brettz9, thank you for your first review! I still need to change a little my last commit to having the full coverage, but I already pushed, so you can see the other changes for now! 😉

Copy link
Collaborator

@brettz9 brettz9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One important and one minor change, though both small and easy-to-change.

All the other recent changes look great.

src/index.js Outdated Show resolved Hide resolved
src/rules/checkLinesAlignment.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@brettz9 brettz9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this one too if you wouldn't mind--forgot there was this...

src/rules/checkLinesAlignment.js Outdated Show resolved Hide resolved
renatho and others added 5 commits September 13, 2020 10:54
"Always" will check for the alignemnt.
"Never" is not yet implemented, but will check if there is no more
than one space between the parts.
Co-authored-by: Brett Zamir <[email protected]>
Report an error through eslint instead of throw.
It's not a standard used by any project, so the default is off
@renatho renatho force-pushed the add/lines-alignment-rule branch from 0d759f2 to 1c1dd68 Compare September 13, 2020 14:07
@renatho
Copy link
Contributor Author

renatho commented Sep 13, 2020

@brettz9 I updated the never temporary warning to be a report instead of a throw. Do you think it makes sense? 38b8b9d

At some point in the future, I'll try to implement the never too. It's easier than the current check. 🙂

@brettz9 brettz9 merged commit 6e2b99d into gajus:master Sep 14, 2020
@gajus
Copy link
Owner

gajus commented Sep 14, 2020

🎉 This PR is included in version 30.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Sep 14, 2020
@brettz9
Copy link
Collaborator

brettz9 commented Sep 14, 2020

Yes, you are correct, it should have been report.

And my apologies, I've just now quickly renamed the rule to check-line-alignment, as a "fix" (line being a generic adjective here). Thought I had merged that version, but hadn't pushed to the branch. I also included the docs in the main README template, removing the need for including the examples (since we auto-generate those from tests).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants