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

chore: update automation process for docusaurus documentation generation #1340

Merged
merged 1 commit into from
Jan 29, 2019

Conversation

jkmdev
Copy link
Contributor

@jkmdev jkmdev commented Jan 25, 2019

Description

This PR adds a script that automates the copying of documentation files into docs/en/packages, a folder that stores package readme pages to be used by Docusaurus. For this script there is a new npm command in the main project's package.json, docs:build.

Also included is this script is a function that handles the execution of copy-package-readme.js's dry-run. For the dry run there is a new npm command in the main project's package.json, docs:dry-run.

To test changes:

  1. In the root folder, run npm run docs:dry-run. The output here should give an overview of what this command will do.
  2. run npm run docs:build. If successful this command will:
    • add to/update all package readme.md files in docs/en/packages
    • add to/update the Packages array in website/sidebars.json
    • add to/update website/i18n/en.json
  3. Switch to the 'website' folder, run npm start. The guide in docusaurus should list links to each package's readme file. Clicking a link will display the relevant readme.

Motivation & context

The readme file for each package needs to be available to docusaurus and this script will make it possible without having to manually copy & paste files every time a readme change is made.

Implements points 3, 4 and 5 in #1312.

Issue type checklist

  • Chore: A change that does not impact distributed packages.
  • Bug fix: A change that fixes an issue, link to the issue above.
  • New feature: A change that adds functionality.

Is this a breaking change?

  • This change causes current functionality to break.
  • This change will override existing files when npm build is run from the websites folder

Process & policy checklist

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

website/package.json Outdated Show resolved Hide resolved
website/sidebars.json Outdated Show resolved Hide resolved
@@ -29,36 +31,130 @@ process.argv.forEach(function (val, index) {
console.log('Invalid argument used. To perform a dry-run use --dry-run');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't catch this last time, but, if we have error messages to show the user if we use strong formatters we can also use color and show those in red.

if (!fs.existsSync(destReadmePath)) {
msg += " ...ADD readme.md into the '" + title + "' folder";
} else {
msg += " ...REPLACE readme.md in the '" + title + "' folder";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer backticks to be used instead of concat +, that would make this something like:

` ...REPLACE readme.md in the '${title}' folder`

srcFiles.forEach((srcReadmePath) => {
createDestReadme(srcReadmePath);
const srcDirPath = path.dirname(srcReadmePath);
sidebarEntries.push("en/packages/" + srcDirPath.split(path.sep).pop() + "/index");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also prefer backticks here

.toLowerCase()
.split(' ')
.map((s) => s.charAt(0).toUpperCase() + s.substring(1))
.join(' ');
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like there's a little bit of inconsistency with the indenting on methods that are added on a newline, can all of these be indented the usual 4 spaces extra so:

var title = srcDirPath
    .split(path.sep)
    .pop()
    // etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yah something felt off about this when I was looking at it, I'll change it to that style for sure.

"id: index\n" +
"title: FAST " + title + '\n' +
"sidebar_label: " + title + '\n' +
"---\n\n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

same request on the indenting here

@codeclimate
Copy link

codeclimate bot commented Jan 29, 2019

Code Climate has analyzed commit 40b9f8e and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 85.4% (0.0% change).

View more on Code Climate.

@nicholasrice
Copy link
Contributor

Thanks @jkmdev!

@jkmdev
Copy link
Contributor Author

jkmdev commented Jan 29, 2019

@nicholasrice No problem! Thanks for the review/feedback!

@awentzel awentzel merged commit f7ff87b into microsoft:master Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants