Build: Edit link in rule docs #9049

j-f1 · 2017-07-31T01:55:53Z

What is the purpose of this pull request?
To add metadata to the rule docs, specifying the edit link for the rules

What changes did you make? (Give an overview)

I added an edit_link key to the YAML front matter of the generated website docs. Additionally, I added progress logging to the npm run gendocs script.

Note: this is a dependency of eslint/archive-website#367.

This also splits the template into separate lines.

eslintbot · 2017-07-31T01:55:54Z

LGTM

mention-bot · 2017-07-31T01:55:58Z

@j-f1, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @mysticatea and @kaicataldo to be potential reviewers.

eslintbot · 2017-07-31T12:04:55Z

LGTM

gyandeeps · 2017-08-04T21:48:29Z

Makefile.js

@@ -668,7 +671,11 @@ target.gensite = function(prereleaseVersion) {
    const FIXABLE_TEXT = "\n\n(fixable) The `--fix` option on the [command line](../user-guide/command-line-interface#fix) can automatically fix some of the problems reported by this rule.";

    // 4. Loop through all files in temporary directory
-    find(TEMP_DIR).forEach(filename => {
+    process.stdout.write("> Updating files (Steps 4-9): 0/... - ...\r");


why not use echo here?

echo adds a \n to the end, which causes the status to extend to multiple lines, which doesn’t provide a very nice experience. Additionally, echo("-n", "foo") doesn’t work.

platinumazure · 2017-08-04T22:01:39Z

Makefile.js

    }

    // 14. Create Example Formatter Output Page
+    echo("> Creating the formatter examples (Step 13)");


Should this be Step 14?

eslintbot · 2017-08-04T22:07:32Z

LGTM

eslintbot · 2017-08-04T22:16:37Z

LGTM

nzakas · 2017-08-23T19:00:26Z

Looks like this was stuck in process. @gyandeeps @platinumazure what are the next steps here?

ilyavolodin · 2017-08-23T19:02:44Z

I think @not-an-aardvark was saying that he things there's a bug in this implementation (that's why we didn't merge it in before the last release).

not-an-aardvark · 2017-08-23T19:04:23Z

Oops, I was planning to review this but I forgot. I'll finish that up soon.

not-an-aardvark

Thanks for the pull request! I left a few questions.

not-an-aardvark · 2017-08-23T19:20:32Z

Makefile.js

+                    "<!-- Note: No pull requests accepted for this file. See README.md in the root directory for details. -->",
+                    "",
+                    text
+                ].filter(x => x).join("\n");


Why is .filter(x => x) used here? It seems like this will result in the "" on line 714 getting removed, which seems redundant.

not-an-aardvark · 2017-08-23T19:20:53Z

Makefile.js

+                    "<!-- Note: No pull requests accepted for this file. See README.md in the root directory for details. -->",
+                    "",
+                    text
+                ].filter(x => x).join("\n");


Why is .filter(x => x) used here? It seems like this will result in the "" on line 733 getting removed, which seems redundant.

not-an-aardvark · 2017-08-23T19:21:24Z

Makefile.js

@@ -745,8 +772,10 @@ target.gensite = function(prereleaseVersion) {
        }
    });
    JSON.stringify(versions).to("./versions.json");
+    echo(`> Updating files (Steps 4-9)${" ".repeat(50)}`);


Why is " ".repeat(50) printed here?

It overwrites any file name contents so they don’t appear after the message.

Makes sense, thanks. To me, It seems undesirable to have a hardcoded value of 50 there (since it will probably cause a bug if we ever add a filepath with length longer than 50), but it's a very small issue either way, so I'm fine with leaving it for now.

not-an-aardvark · 2017-08-23T19:23:57Z

Makefile.js

@@ -680,6 +687,8 @@ target.gensite = function(prereleaseVersion) {
            let text = cat(filename),
                title;

+            process.stdout.write(`> Updating files (Steps 4-9): ${i}/${length} - ${sourcePath + " ".repeat(30)}\r`);


Why is \r used here instead of \n?

It resets to the beginning of the line, instead of spamming the terminal with lots and lots of lines.

not-an-aardvark · 2017-08-23T19:27:55Z

Makefile.js

+                    "---",
+                    `title: ${title}`,
+                    "layout: doc",
+                    `edit_link: https://github.com/eslint/eslint/edit/master/${baseName}`,


Should this use filename instead of baseName? Otherwise, I think a file like docs/about/index.md will map to a link like https://github.com/eslint/eslint/edit/master/index.md, resulting in a 404.

platinumazure

My concerns are addressed, very sorry for not following up sooner. Thanks for contributing!

eslintbot · 2017-08-25T23:13:14Z

LGTM

j-f1 · 2017-08-25T23:14:59Z

@not-an-aardvark I think I’ve resolved the issues you’ve raised.

eslintbot · 2017-08-25T23:20:38Z

LGTM

not-an-aardvark

Thanks, this looks good to me. I have a few more questions, but none of them are blockers for merging this.

not-an-aardvark · 2017-08-26T00:48:44Z

Makefile.js

-                text = `---\ntitle: ${ruleName} - Rules\nlayout: doc\n---\n<!-- Note: No pull requests accepted for this file. See README.md in the root directory for details. -->\n\n${text}`;
+                title = `${ruleName} - Rules`;
+
+                editLink = `https://github.com/eslint/eslint/edit/master/docs/rules/${baseName}`;


Nitpick: Is it necessary to assign editLink separately when the page is in the rules directory? Wouldn't `https://github.com/eslint/eslint/edit/master/${filePath}` work in both cases?

Awesome! I didn’t notice that.

not-an-aardvark · 2017-08-26T00:48:49Z

Makefile.js

+                "---",
+                `title: ${title}`,
+                "layout: doc",
+                editLink ? `edit_link: ${editLink}` : "",


Nitpick: Will editLink ever be falsy here? It seems like it gets assigned on every code path.

You’re right, removed the conditional.

not-an-aardvark · 2017-08-26T00:48:55Z

Makefile.js

+                title,
+                editLink;
+
+            process.stdout.write(`> Updating files (Steps 4-9): ${i}/${length} - ${filePath + " ".repeat(30)}\n`);


I noticed you changed this back to \n rather than \r -- was that intentional? If so, should the " ".repeat(30) be removed?

I changed it for testing the update, but I forgot to change it back. Fixed!

not-an-aardvark · 2017-08-26T00:51:57Z

Makefile.js

@@ -745,8 +772,10 @@ target.gensite = function(prereleaseVersion) {
        }
    });
    JSON.stringify(versions).to("./versions.json");
+    echo(`> Updating files (Steps 4-9)${" ".repeat(50)}`);


Makes sense, thanks. To me, It seems undesirable to have a hardcoded value of 50 there (since it will probably cause a bug if we ever add a filepath with length longer than 50), but it's a very small issue either way, so I'm fine with leaving it for now.

eslintbot · 2017-08-26T00:58:11Z

LGTM

eslintbot · 2017-08-26T01:07:59Z

LGTM

not-an-aardvark · 2017-08-26T20:54:32Z

Thanks for contributing!

j-f1 added 2 commits July 30, 2017 21:50

Build: Add an edit link to the rule docs’ metadata

d18c3e2

This also splits the template into separate lines.

Build: add progress details to the gendocs script

facb3f1

j-f1 added a commit to j-f1/forked-eslint.github.io that referenced this pull request Jul 31, 2017

Update the docs based on eslint/eslint#9049

c394f3e

j-f1 mentioned this pull request Jul 31, 2017

Add a 404 page and an “Edit this page” link, as well as some other polish eslint/archive-website#367

Merged

j-f1 changed the title ~~Edit link in rule docs~~ Build: Edit link in rule docs Jul 31, 2017

Follow the style guide

fcf249e

not-an-aardvark added build This change relates to ESLint's build process documentation Relates to ESLint's documentation labels Aug 1, 2017

gyandeeps reviewed Aug 4, 2017

View reviewed changes

platinumazure reviewed Aug 4, 2017

View reviewed changes

Fix errors in edit links

7c110e0

Typos

bf28389

not-an-aardvark self-requested a review August 23, 2017 19:04

not-an-aardvark reviewed Aug 23, 2017

View reviewed changes

platinumazure approved these changes Aug 23, 2017

View reviewed changes

Dedupe the header logic; correct edit link for rules

8cdeaae

not-an-aardvark approved these changes Aug 26, 2017

View reviewed changes

j-f1 added 2 commits August 25, 2017 20:55

Correct the edit link *again*

e470cb4

Simplify the edit link generation

6b129e0

j-f1 force-pushed the edit-link-in-rule-docs branch from 4c2ae71 to 6b129e0 Compare August 26, 2017 00:58

🔥 unused variable

7442f55

not-an-aardvark approved these changes Aug 26, 2017

View reviewed changes

not-an-aardvark merged commit e6b115c into eslint:master Aug 26, 2017

j-f1 deleted the edit-link-in-rule-docs branch August 26, 2017 21:45

eslint-deprecated bot locked and limited conversation to collaborators Feb 23, 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 23, 2018

Build: Edit link in rule docs #9049

Build: Edit link in rule docs #9049

Conversation

j-f1 commented Jul 31, 2017

eslintbot commented Jul 31, 2017

mention-bot commented Jul 31, 2017

eslintbot commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Aug 4, 2017

eslintbot commented Aug 4, 2017

nzakas commented Aug 23, 2017

ilyavolodin commented Aug 23, 2017

not-an-aardvark commented Aug 23, 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

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

not-an-aardvark Aug 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platinumazure left a comment

Choose a reason for hiding this comment

eslintbot commented Aug 25, 2017

j-f1 commented Aug 25, 2017

eslintbot commented Aug 25, 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

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 Aug 26, 2017

eslintbot commented Aug 26, 2017

not-an-aardvark commented Aug 26, 2017

not-an-aardvark Aug 23, 2017 •

edited