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: add script for copying package readme files to docs/en/packages #1315

Merged
merged 1 commit into from Jan 24, 2019

Conversation

Projects
None yet
5 participants
@jkmdev
Copy link
Contributor

commented Jan 17, 2019

Description

Adds a script to build/documentation that will copy all package readme.md files to docs/en/packages.
Run node build/documentation/copy-package-readme.js to see its results.

Motivation & context

Incremental addition to #1312.
Implements points 1 and 2.

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.

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.
@awentzel

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

I tried running this on my system (Windows) that has two folders already existing and it rewrote those which is great (though I did lose the header of the files in my case since I had templated out steps you have not yet gotten too so not a big deal)

But, the script didn't create any of the folders or files for all the other packages?

Is there special documentation notes we should add? How did you test? Should we add a parameter for --dry-run to pretest since this will ultimately be ran as part of a another script, for example, building the Docusaurus site.

Error: ENOENT: no such file or directory, mkdir 'D:\projects\fast-dna\docs/en/packages\fast-breakpoint-tracker-react\README.md'

@jkmdev jkmdev force-pushed the jkmdev:issue-1312 branch from 77b7619 to e3a9f77 Jan 18, 2019

@jkmdev

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

There was that error because I wrote this in Linux, my use of forward-slashes in some places made it so that the filepaths weren't recognized by Windows.

I made the fix and tested it in Windows, it should work now after running node build/documentation/copy-package-readme.js.

To test I've just been running node build/documentation/copy-package-readme.js and checking if it has the desired output.

--dry-run? So for this I'd print to the console what the script is doing without creating folders or files? Seems simple, I can add it if you want. I'm not 100% sure about its utility, but it seems like a good idea since it's a process that will overwrite all related files.

@Falkicon

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

@jkmdev, this was something I really wanted to see done for the docs but we had not gotten to it yet. Thank you for the contribution!

@awentzel

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

—dry-run would be great. We will want the scripts to run on both Windows and Mac. Ping me on Discord and I’ll take another look when it’s ready and do another round of testing.

@jkmdev

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

@Falkicon Happy to help!
@awentzel Alright, will do.

@jkmdev jkmdev force-pushed the jkmdev:issue-1312 branch 3 times, most recently from be2d021 to b0ea3fd Jan 19, 2019

@awentzel
Copy link
Member

left a comment

Works great on Windows now including the --dry-run. But, regressed on Mac. --dry-run on mac works.

fs.js:909
  return binding.mkdir(pathModule.toNamespacedPath(path),
                 ^

Error: ENOENT: no such file or directory, mkdir '/Users/aaronw/aWork/Microsoft/Projects/FAST/fast-dna/docs\en\packages/fast-animation/README.md'
    at Object.fs.mkdirSync (fs.js:909:18)
    at files.forEach (/Users/aaronw/aWork/Microsoft/Projects/FAST/fast-dna/build/documentation/copy-package-readme.js:45:83)
    at Array.forEach (<anonymous>)
    at /Users/aaronw/aWork/Microsoft/Projects/FAST/fast-dna/build/documentation/copy-package-readme.js:41:15
    at f (/Users/aaronw/aWork/Microsoft/Projects/FAST/fast-dna/node_modules/once/once.js:25:25)
    at Glob.<anonymous> (/Users/aaronw/aWork/Microsoft/Projects/FAST/fast-dna/node_modules/glob/glob.js:151:7)
    at Glob.emit (events.js:180:13)
    at Glob._finish (/Users/aaronw/aWork/Microsoft/Projects/FAST/fast-dna/node_modules/glob/glob.js:197:8)
    at next (/Users/aaronw/aWork/Microsoft/Projects/FAST/fast-dna/node_modules/glob/glob.js:216:12)
    at /Users/aaronw/aWork/Microsoft/Projects/FAST/fast-dna/node_modules/glob/glob.js:248:9

files.forEach((filePath) => {
const destReadmePath = filePath.replace(/(\bpackages\b)(?!.*\1)/, destDir);
const dirPath = destReadmePath.replace(/(\b\\README.md\b)(?!.*\1)/, '');

This comment has been minimized.

Copy link
@nicholasrice

nicholasrice Jan 22, 2019

Member

Instead of replacing "README.md" I would instead utilize Node's API - const dirPath = path.dirname(destReadmePath) - I've found using the supported API's for path manipulation really helps minimize platform bugs.


const rootDir = path.resolve(process.cwd());
const srcReadmePaths = "packages/*/README.md";
const destDir = "docs\\en\\packages";

This comment has been minimized.

Copy link
@nicholasrice

nicholasrice Jan 22, 2019

Member

this destDir is now causing the script to throw in my environment.

I would recommend using path.join("docs", "en", "packages"). Alternatively, you can construct the path using path.sep to easily allow Node to pick the path separator based on runtime environment.

This comment has been minimized.

Copy link
@jkmdev

jkmdev Jan 23, 2019

Author Contributor

Ahhh this feedback was very helpful, tyty

This comment has been minimized.

Copy link
@nicholasrice

nicholasrice Jan 23, 2019

Member

Of course! Thanks for making the update :)

@jkmdev jkmdev force-pushed the jkmdev:issue-1312 branch from b0ea3fd to bd9dff3 Jan 23, 2019

@jkmdev jkmdev force-pushed the jkmdev:issue-1312 branch from bd9dff3 to e508d21 Jan 24, 2019

@codeclimate

This comment has been minimized.

Copy link

commented Jan 24, 2019

Code Climate has analyzed commit e508d21 and detected 0 issues on this pull request.

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.3% (0.0% change).

View more on Code Climate.

@awentzel awentzel merged commit 7d93056 into Microsoft:master Jan 24, 2019

6 checks passed

ci/circleci: build_dependencies Your tests passed on CircleCI!
Details
ci/circleci: test_coverage Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/diff-coverage 100% (50% threshold)
Details
codeclimate/total-coverage 85% (0.0% change)
Details
license/cla All CLA requirements met.
Details

@jkmdev jkmdev deleted the jkmdev:issue-1312 branch Jan 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.