Build: display rules’ meta data in their docs (fixes #5774) #8127

wkurniawan07 · 2017-02-22T04:57:24Z

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

[X] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Modified the generation of eslint.github.io website so that important rule metadata such as recommended and fixable are automatically appended to the documentation of each rule.

This removes the need to hard-code the "fixable" portion on all fixable rules.

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

Nothing in particular.

eslintbot · 2017-02-22T04:57:25Z

LGTM

mention-bot · 2017-02-22T04:57:26Z

@wkurniawan07, thanks for your PR! By analyzing the history of the files in this pull request, we identified @not-an-aardvark, @madmed88 and @nzakas to be potential reviewers.

wkurniawan07 · 2017-02-22T04:58:51Z

Sample rule page (ran locally):

not-an-aardvark

This looks great! I just have a few suggestions.

This is reminding me that we should really add tests for some of the functionality in Makefile.js. But that doesn't have to be a part of this PR.

not-an-aardvark · 2017-02-22T05:01:05Z

Makefile.js

+        const rule = require(filename);
+        const ruleName = path.basename(filename, ".js");
+
+        if (rule.meta.docs.recommended || false) {


It looks like you left a || false from debugging.

not-an-aardvark · 2017-02-22T05:09:06Z

Makefile.js

+    const fixable = [];
+
+    find(path.join(process.cwd(), "/lib/rules/")).filter(fileType("js")).forEach(filename => {
+        const rule = require(filename);


Would it be possible to use require('.').linter.getRules() to get all the rules, instead of looking through the filesystem?

not-an-aardvark · 2017-02-22T05:11:15Z

Makefile.js

+
+                // Find out if the rule requires a special docs portion (e.g. if it is recommended and/or fixable)
+                const isRecommended = recommended.indexOf(ruleName) >= 0;
+                const isFixable = fixable.indexOf(ruleName) >= 0;


It doesn't really matter since there are a fairly small number of rules, but it might be better to use a Set for recommended and fixable since we don't care about the ordering here.

eslintbot · 2017-02-22T05:43:29Z

LGTM

wkurniawan07 · 2017-02-22T05:49:26Z

@not-an-aardvark thanks for the prompt review! Made all the changes as suggested.

About the initial usage of find(path.join(...))... to find the rules in the file system, I actually followed the idea from this part of the very same file.

eslintbot · 2017-02-22T18:31:16Z

LGTM

not-an-aardvark · 2017-02-22T18:35:13Z

About the initial usage of find(path.join(...))... to find the rules in the file system, I actually followed the idea from this part of the very same file.

Fair enough. The linter.getRules() API was only introduced recently, so we probably haven't taken full advantage of it everywhere yet.

not-an-aardvark

LGTM. Thanks!

vitorbal

Thanks, great job on this! LGTM

ilyavolodin

I think we don't need Sets. They just add new loop and consume more memory.

ilyavolodin · 2017-02-23T02:54:32Z

Makefile.js

+        }
+    });
+
+    const RECOMMENDED_TEXT = "\n\n(recommended) The `\"extends\": \"eslint:recommended\"` property in a configuration file enables this rule.";


I wonder if we should just append HTML directly, without having to use replace later? We currently replace (recommended) and (fixable) with markup in Jekyll. @eslint/eslint-team thoughts?

ilyavolodin · 2017-02-23T02:59:30Z

Makefile.js

            if (path.dirname(filename).indexOf("rules") >= 0) {
+
+                // Find out if the rule requires a special docs portion (e.g. if it is recommended and/or fixable)
+                const isRecommended = recommended.has(ruleName);


I don't think we need to use Set for both of them? Maybe we should do rules.get(ruleName).meta.docs.recommended instead?

Good point -- I suppose there's no need to do this in a separate loop.

Gah, I just noticed that you actually requested adding Sets. Sorry I didn't look at this earlier:-( I still think adding an extra loop and two new data structures that we are not going to use for anything else later is an overkill, but I guess I can live with this, if everyone else thinks that it's fine.

I'm convinced by your argument and I agree that it's overkill. @wkurniawan07, apologies for the misleading review earlier.

@not-an-aardvark no worries, we all make mistakes sometimes.
@ilyavolodin will change as suggested, it does make the script a lot shorter as well!

ilyavolodin · 2017-02-23T03:13:16Z

Also this should be Build PR, not Docs, since it modifies build script.

eslintbot · 2017-02-23T04:28:26Z

LGTM

ilyavolodin

Awesome. LGTM, thanks.

alberto · 2017-02-23T18:11:14Z

Makefile.js

+    const rules = require(".").linter.getRules();
+
+    const RECOMMENDED_TEXT = "\n\n(recommended) The `\"extends\": \"eslint:recommended\"` property in a configuration file enables this rule.";
+    const FIXABLE_TEXT = "\n\n(fixable) The `--fix` option on the [command line](../user-guide/command-line-interface#fix) automatically fixes problems reported by this rule.";


Can we change the text to:

"\n\n(fixable) The --fix option on the command line can automatically fix some of the problems reported by this rule.";

@alberto Done! 🙂

eslintbot · 2017-02-23T18:31:22Z

LGTM

wkurniawan07 · 2017-02-23T18:48:40Z

Thanks for all the support! I'll see if I can contribute to more issues. 🙂

not-an-aardvark suggested changes Feb 22, 2017

View reviewed changes

wkurniawan07 force-pushed the Issue5774 branch from 204fa5f to c92dbc9 Compare February 22, 2017 05:43

wkurniawan07 force-pushed the Issue5774 branch from c92dbc9 to 59af1ee Compare February 22, 2017 18:31

not-an-aardvark approved these changes Feb 22, 2017

View reviewed changes

vitorbal approved these changes Feb 22, 2017

View reviewed changes

ilyavolodin suggested changes Feb 23, 2017

View reviewed changes

ilyavolodin added accepted There is consensus among the team that this change meets the criteria for inclusion build This change relates to ESLint's build process enhancement This change enhances an existing feature of ESLint labels Feb 23, 2017

wkurniawan07 changed the title ~~Docs: display rules’ meta data in their docs (fixes #5774)~~ Build: display rules’ meta data in their docs (fixes #5774) Feb 23, 2017

wkurniawan07 force-pushed the Issue5774 branch from 59af1ee to aefbf32 Compare February 23, 2017 04:28

ilyavolodin approved these changes Feb 23, 2017

View reviewed changes

platinumazure approved these changes Feb 23, 2017

View reviewed changes

alberto suggested changes Feb 23, 2017

View reviewed changes

Build: display rules’ meta data in their docs (fixes eslint#5774)

1dd0054

wkurniawan07 force-pushed the Issue5774 branch from aefbf32 to 1dd0054 Compare February 23, 2017 18:31

alberto approved these changes Feb 23, 2017

View reviewed changes

alberto merged commit cbd7ded into eslint:master Feb 23, 2017

wkurniawan07 deleted the Issue5774 branch February 23, 2017 18:48

This was referenced Mar 3, 2017

Update dependency eslint to version 3.19.0 singapore/lint-condo#244

Merged

Update dependency eslint to version 3.17.1 renovatebot/renovate#123

Merged

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

Build: display rules’ meta data in their docs (fixes #5774) #8127

Build: display rules’ meta data in their docs (fixes #5774) #8127

wkurniawan07 commented Feb 22, 2017

eslintbot commented Feb 22, 2017

mention-bot commented Feb 22, 2017

wkurniawan07 commented Feb 22, 2017

not-an-aardvark left a comment

not-an-aardvark Feb 22, 2017

not-an-aardvark Feb 22, 2017

not-an-aardvark Feb 22, 2017

eslintbot commented Feb 22, 2017

wkurniawan07 commented Feb 22, 2017

eslintbot commented Feb 22, 2017

not-an-aardvark commented Feb 22, 2017

not-an-aardvark left a comment

vitorbal left a comment

ilyavolodin left a comment

ilyavolodin Feb 23, 2017

ilyavolodin Feb 23, 2017

not-an-aardvark Feb 23, 2017

ilyavolodin Feb 23, 2017

not-an-aardvark Feb 23, 2017

wkurniawan07 Feb 23, 2017 •

edited

ilyavolodin commented Feb 23, 2017

eslintbot commented Feb 23, 2017

ilyavolodin left a comment

alberto Feb 23, 2017

wkurniawan07 Feb 23, 2017

eslintbot commented Feb 23, 2017

wkurniawan07 commented Feb 23, 2017

Build: display rules’ meta data in their docs (fixes #5774) #8127

Build: display rules’ meta data in their docs (fixes #5774) #8127

Conversation

wkurniawan07 commented Feb 22, 2017

eslintbot commented Feb 22, 2017

mention-bot commented Feb 22, 2017

wkurniawan07 commented Feb 22, 2017

not-an-aardvark left a 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

eslintbot commented Feb 22, 2017

wkurniawan07 commented Feb 22, 2017

eslintbot commented Feb 22, 2017

not-an-aardvark commented Feb 22, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

vitorbal left a comment

Choose a reason for hiding this comment

ilyavolodin left a 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

wkurniawan07 Feb 23, 2017 • edited

Choose a reason for hiding this comment

ilyavolodin commented Feb 23, 2017

eslintbot commented Feb 23, 2017

ilyavolodin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Feb 23, 2017

wkurniawan07 commented Feb 23, 2017

wkurniawan07 Feb 23, 2017 •

edited