Chore: add internal rule to enforce meta.docs conventions (fixes #6954) #7155

vitorbal · 2016-09-14T17:24:59Z

What is the purpose of this pull request? (put an "X" next to item)

[x] Other, please explain:
Chore. Add a new internal rule to enforce meta.docs.description conventions.

Please check each item to ensure your pull request is ready:

I've read the pull request guide
I've included tests for my change
I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)
I created a new internal rule to enforce that the meta.docs.description property in our core rules follow our conventions of starting with a lowercase character and with one of the following words: "enforce", "disallow", "require". See #6954 for more.

Is there anything you'd like reviewers to focus on?

is the name of the rule okay?
any more test cases that you can think of?

…nt#6954)

mention-bot · 2016-09-14T17:25:00Z

@vitorbal, thanks for your PR! By analyzing the annotation information on this pull request, we identified @mysticatea and @kaicataldo to be potential reviewers

eslintbot · 2016-09-14T17:25:00Z

LGTM

platinumazure · 2016-09-14T17:33:10Z

I'd love to see some invalid tests around Require/Enforce/Disallow (not lowercase). Otherwise LGTM!

platinumazure · 2016-09-14T17:34:09Z

Oh, and as a dev team member on ESLint, you can (and probably should) do your work on branches in this repo rather than your fork 😄

eslintbot · 2016-09-14T22:31:01Z

LGTM

platinumazure · 2016-09-15T01:45:23Z

LGTM, thanks!

gyandeeps · 2016-09-15T01:53:33Z

lib/internal-rules/internal-consistent-docs-description.js

+    }
+
+    const description = metaDocsDescription.value.value;
+    const firstWord = description.split(" ")[0];


What if the description is
" requires spaces"

@gyandeeps That's a good point, thanks! I pushed a new commit that uses a regular expression to try to get the first word instead of relying on .split(). I also added some extra tests for that. Let me know what you think!

eslintbot · 2016-09-17T19:16:20Z

LGTM

kaicataldo · 2016-09-18T01:18:45Z

lib/internal-rules/internal-consistent-docs-description.js

+ * @returns {void}
+ */
+function checkMetaDocsDescription(context, exportsNode) {
+    if (!isCorrectExportsFormat(exportsNode)) {


The function name as it stands now makes it sound like it's actually checking the structure of the exported object. Maybe something like isObjectExpression() would be better? In fact, any reason why we can't just inline exportsNode.type === "ObjectExpression" since it's only called once and only does that check?

I think inlining it makes sense here. Thanks for the suggestion!

kaicataldo · 2016-09-18T01:36:19Z

lib/internal-rules/internal-consistent-docs-description.js

+
+    const description = metaDocsDescription.value.value;
+    const matchFirstWord = FIRST_WORD_REGEX.exec(description);
+    const firstWord = (matchFirstWord && matchFirstWord[1]) ? matchFirstWord[1] : description;


I wonder if it might be a bit clearer if this line was const firstWord = matchFirstWord !== null ? matchFirstWord[1] : description;

Good catch, the check for matchFirstWord[1] is not needed. Thanks! :)

kaicataldo · 2016-09-18T01:42:41Z

tests/lib/internal-rules/internal-consistent-docs-description.js

+                "module.exports = {",
+                "    meta: {",
+                "        docs: {",
+                "            description: ''",


I don't think this is a blocker, but something to consider - do we want to have an alternate error message if the description field is empty or is just whitespace? Might not be necessary since this is entirely internal and we'll know what this means :)

What if we have two separate messages for:

description is empty: 'meta.docs.description' must not be empty.

description starts with a whitespace (should cover "all whitespace" case as well): meta.docs.description should not start with whitespaces.

That way we can also revert the regex changes and simplify the "firstWord" check to use .split() again.
What do you think?

I like that - I think those are more helpful error messages!

Added the additional reports. Let me know what you think :)

kaicataldo · 2016-09-18T01:51:30Z

tests/lib/internal-rules/internal-consistent-docs-description.js

+                column: 26
+            }]
+        },
+        {


Looks like this might be a duplicate of the test on line 103

You're right, thanks!

kaicataldo · 2016-09-18T01:51:46Z

tests/lib/internal-rules/internal-consistent-docs-description.js

+                column: 26
+            }]
+        },
+        {


This could potentially be warned by a separate error if the description is empty or just whitespace.

kaicataldo · 2016-09-18T01:54:33Z

Yikes - looks like some bugs with the review comments - the last two are showing up on the correct lines in the Conversation view but are in the wrong place in the Files changed view

eslintbot · 2016-09-19T16:26:40Z

LGTM

kaicataldo · 2016-09-19T17:17:43Z

lib/internal-rules/internal-consistent-docs-description.js

+    if (description.indexOf(" ") === 0) {
+        context.report({
+            node: metaDocsDescription.value,
+            message: "`meta.docs.description` should not start with whitespaces."


I think we use whitespace to refer to both single or multiple whitespace characters in most of our other documentation, so maybe this could be "meta.docs.description should not start with whitespace."?

kaicataldo · 2016-09-19T17:20:47Z

tests/lib/internal-rules/internal-consistent-docs-description.js

+ruleTester.run("internal-consistent-docs-description", rule, {
+    valid: [
+
+        // wrong exports format: "internal-no-valid-meta" reports this already


Typo in comment: internal-no-valid-meta -> internal-no-invalid-meta

kaicataldo · 2016-09-19T17:20:52Z

tests/lib/internal-rules/internal-consistent-docs-description.js

+            "};"
+        ].join("\n"),
+
+        // missing `meta.docs.description` property: "internal-no-valid-meta" reports this already


Typo in comment: internal-no-valid-meta -> internal-no-invalid-meta

eslintbot · 2016-09-26T08:20:23Z

LGTM

vitorbal · 2016-09-26T08:21:53Z

@kaicataldo sorry for the delay. Thanks again for all the good feedback, I really appreciate it! Just pushed a new commit that fixes the typos you mentioned.

kaicataldo

LGTM, but waiting for today's patch release before merging

kaicataldo · 2016-09-27T14:47:05Z

Thanks for working on this!

Chore: add internal rule to enforce meta.docs conventions (fixes esli…

342e3b7

…nt#6954)

jquerybot added the CLA: Valid label Sep 14, 2016

vitorbal mentioned this pull request Sep 14, 2016

Add a custom rule to enforce conventions in rule metadata #6954

Closed

Chore: add more tests for non-lowercase

8b998a2

gyandeeps reviewed Sep 15, 2016

View reviewed changes

Chore: add more tests and check for leading whitespaces

f835537

ilyavolodin approved these changes Sep 17, 2016

View reviewed changes

kaicataldo reviewed Sep 18, 2016

View reviewed changes

vitorbal added the accepted There is consensus among the team that this change meets the criteria for inclusion label Sep 19, 2016

Chore: add separate reports for whitespace issues

f847c52

kaicataldo reviewed Sep 19, 2016

View reviewed changes

Chore: fix typos

69f71e7

kaicataldo approved these changes Sep 26, 2016

View reviewed changes

kaicataldo merged commit 7bb800d into eslint:master Sep 27, 2016

vitorbal deleted the metadata_internal_rule branch September 27, 2016 15:41

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: add internal rule to enforce meta.docs conventions (fixes #6954) #7155

Chore: add internal rule to enforce meta.docs conventions (fixes #6954) #7155

vitorbal commented Sep 14, 2016

mention-bot commented Sep 14, 2016

eslintbot commented Sep 14, 2016

platinumazure commented Sep 14, 2016 •

edited

platinumazure commented Sep 14, 2016

eslintbot commented Sep 14, 2016

platinumazure commented Sep 15, 2016

gyandeeps Sep 15, 2016

vitorbal Sep 17, 2016

eslintbot commented Sep 17, 2016

kaicataldo Sep 18, 2016 •

edited

vitorbal Sep 19, 2016

kaicataldo Sep 18, 2016 •

edited

vitorbal Sep 19, 2016

kaicataldo Sep 18, 2016 •

edited

vitorbal Sep 19, 2016

kaicataldo Sep 19, 2016

vitorbal Sep 19, 2016

kaicataldo Sep 18, 2016

vitorbal Sep 19, 2016

kaicataldo Sep 18, 2016

kaicataldo commented Sep 18, 2016

eslintbot commented Sep 19, 2016

kaicataldo Sep 19, 2016

kaicataldo Sep 19, 2016

kaicataldo Sep 19, 2016

eslintbot commented Sep 26, 2016

vitorbal commented Sep 26, 2016

kaicataldo left a comment

kaicataldo commented Sep 27, 2016

Chore: add internal rule to enforce meta.docs conventions (fixes #6954) #7155

Chore: add internal rule to enforce meta.docs conventions (fixes #6954) #7155

Conversation

vitorbal commented Sep 14, 2016

mention-bot commented Sep 14, 2016

eslintbot commented Sep 14, 2016

platinumazure commented Sep 14, 2016 • edited

platinumazure commented Sep 14, 2016

eslintbot commented Sep 14, 2016

platinumazure commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Sep 17, 2016

kaicataldo Sep 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo Sep 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo Sep 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo commented Sep 18, 2016

eslintbot commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Sep 26, 2016

vitorbal commented Sep 26, 2016

kaicataldo left a comment

Choose a reason for hiding this comment

kaicataldo commented Sep 27, 2016

platinumazure commented Sep 14, 2016 •

edited

kaicataldo Sep 18, 2016 •

edited

kaicataldo Sep 18, 2016 •

edited

kaicataldo Sep 18, 2016 •

edited