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.

Sign up for GitHub

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

Jump to bottom

Build: Edit link in rule docs #9049

Merged
merged 9 commits into from Aug 26, 2017
Merged

Build: Edit link in rule docs #9049

Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d18c3e2
Build: Add an edit link to the rule docs’ metadata
j-f1 Jul 31, 2017
facb3f1
Build: add progress details to the gendocs script
j-f1 Jul 31, 2017
fcf249e
Follow the style guide
j-f1 Jul 31, 2017
7c110e0
Fix errors in edit links
j-f1 Aug 4, 2017
bf28389
Typos
j-f1 Aug 4, 2017
8cdeaae
Dedupe the header logic; correct edit link for rules
j-f1 Aug 25, 2017
e470cb4
Correct the edit link *again*
j-f1 Aug 25, 2017
6b129e0
Simplify the edit link generation
j-f1 Aug 26, 2017
7442f55
🔥 unused variable
j-f1 Aug 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 40 additions & 3 deletions Makefile.js
View file
Open in desktop
Expand Up @@ -631,11 +631,13 @@ target.gensite = function(prereleaseVersion) {
}

// 1. create temp and build directory
echo("> Creating a temporary directory (Step 1)");
if (!test("-d", TEMP_DIR)) {
mkdir(TEMP_DIR);
}

// 2. remove old files from the site
echo("> Removing old files (Step 2)");
docFiles.forEach(filePath => {
const fullPath = path.join(DOCS_DIR, filePath),
htmlFullPath = fullPath.replace(".md", ".html");
Expand All @@ -651,6 +653,7 @@ target.gensite = function(prereleaseVersion) {
});

// 3. Copy docs folder to a temporary directory
echo("> Copying the docs folder (Step 3)");
cp("-rf", "docs/*", TEMP_DIR);

let versions = test("-f", "./versions.json") ? JSON.parse(cat("./versions.json")) : {};
Expand All @@ -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");
Copy link
Member

Choose a reason for hiding this comment

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

why not use echo here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const tempFiles = find(TEMP_DIR);
const length = tempFiles.length;

tempFiles.forEach((filename, i) => {
if (test("-f", filename) && path.extname(filename) === ".md") {

const rulesUrl = "https://github.com/eslint/eslint/tree/master/lib/rules/",
Expand All @@ -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`);
Copy link
Member

Choose a reason for hiding this comment

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

Why is \r used here instead of \n?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Neat, TIL


// 5. Prepend page title and layout variables at the top of rules
if (path.dirname(filename).indexOf("rules") >= 0) {

Expand All @@ -695,7 +704,16 @@ target.gensite = function(prereleaseVersion) {

text = `${ruleHeading}${isRecommended ? RECOMMENDED_TEXT : ""}${isFixable ? FIXABLE_TEXT : ""}\n${ruleDocsContent}`;

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}`;
text = [
"---",
`title: ${ruleName} - Rules`,
"layout: doc",
filename.indexOf("rules/") !== -1 && `edit_link: https://github.com/eslint/eslint/edit/master/docs/rules/${baseName}`,
"---",
"<!-- Note: No pull requests accepted for this file. See README.md in the root directory for details. -->",
"",
text
].filter(x => x).join("\n");
Copy link
Member

Choose a reason for hiding this comment

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

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

} else {

// extract the title from the file itself
Expand All @@ -705,7 +723,16 @@ target.gensite = function(prereleaseVersion) {
} else {
title = "Documentation";
}
text = `---\ntitle: ${title}\nlayout: doc\n---\n<!-- Note: No pull requests accepted for this file. See README.md in the root directory for details. -->\n\n${text}`;
text = [
"---",
`title: ${title}`,
"layout: doc",
`edit_link: https://github.com/eslint/eslint/edit/master/${baseName}`,
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

}

// 6. Remove .md extension for relative links and change README to empty string
Expand Down Expand Up @@ -745,8 +772,10 @@ target.gensite = function(prereleaseVersion) {
}
});
JSON.stringify(versions).to("./versions.json");
echo(`> Updating files (Steps 4-9)${" ".repeat(50)}`);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.


// 10. Copy temporary directory to site's docs folder
echo("> Copying the temporary directory the site (Step 10)");
let outputDir = DOCS_DIR;

if (prereleaseVersion) {
Expand All @@ -755,18 +784,26 @@ target.gensite = function(prereleaseVersion) {
cp("-rf", `${TEMP_DIR}*`, outputDir);

// 11. Generate rule listing page
echo("> Generating the rule listing (Step 11)");
generateRuleIndexPage(process.cwd());

// 12. Delete temporary directory
echo("> Removing the temporary directory (Step 12)");
rm("-r", TEMP_DIR);

// 13. Update demos, but only for non-prereleases
if (!prereleaseVersion) {
echo("> Updating the demos (Step 13)");
cp("-f", "build/eslint.js", `${SITE_DIR}js/app/eslint.js`);
} else {
echo("> Skipped updating the demos (Step 13)");
}

// 14. Create Example Formatter Output Page
echo("> Creating the formatter examples (Step 14)");
generateFormatterExamples(getFormatterResults(), prereleaseVersion);

echo("Done generating eslint.org");
};

target.browserify = function() {
Expand Down