Fix #82: Providing the motivation and how to use this guide#281
Fix #82: Providing the motivation and how to use this guide#281calcium-wq wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the project's onboarding documentation by adding descriptive sections to the README and playground documentation regarding the guide's scope and usage. It also introduces a new test suite to verify README content and updates the package.json scripts, notably disabling the doctoc automation. Feedback highlights a contradiction in the doctoc setup where markers and dependencies remain despite the script being disabled. Furthermore, it is recommended to use more resilient regex patterns in the new tests to avoid brittleness against minor documentation changes.
| "doctoc": "doctoc --maxlevel=3 README_SOURCE.md", | ||
| "test": "node --test test/*.test.js", | ||
| "ci-check": "npm run readme:generate && npm test", | ||
| "doctoc": "node -e \"console.log('README_SOURCE.md TOC is maintained in source for this repository.')\"", |
There was a problem hiding this comment.
The doctoc script has been replaced with a log message, effectively disabling automatic Table of Contents generation. However, the README.md and README_SOURCE.md files still contain the <!-- START doctoc --> markers and instructions to run doctoc for updates. This creates a contradiction for contributors. Additionally, the doctoc package remains in devDependencies. If manual maintenance is the goal, please remove the markers and the dependency. If not, consider restoring the script's functionality.
| test('README explains what the guide is and is not', () => { | ||
| assert.match( | ||
| readme, | ||
| /### \*\*What This Guide Is\*\*/ | ||
| ); | ||
| assert.match( | ||
| readme, | ||
| /This repository is a reference guide and recipe collection for using React, Redux, and TypeScript together\./ | ||
| ); | ||
| assert.match( | ||
| readme, | ||
| /It is \*\*not\*\* a boilerplate, starter kit, or library that introduces its own replacement APIs for React\./ | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This test matches long, exact strings from the README, making it brittle to minor documentation changes (e.g., rephrasing or typo fixes). Consider using more resilient matches for key phrases or headers to improve maintainability.
test('README explains what the guide is and is not', () => {
assert.match(readme, /### \*\*What This Guide Is\*\*/);
assert.match(readme, /reference guide and recipe collection/);
assert.match(readme, /not\*\* a boilerplate/);
});| test('README gives beginners a reading path and clarifies the styleguide', () => { | ||
| assert.match( | ||
| readme, | ||
| /### \*\*How to Use This Guide\*\*/ | ||
| ); | ||
| assert.match( | ||
| readme, | ||
| /Start with the \[Playground Project\]\(#playground-project\) to see where the examples live\./ | ||
| ); | ||
| assert.match( | ||
| readme, | ||
| /### \*\*Understanding the Playground and Style Guide\*\*/ | ||
| ); | ||
| assert.match( | ||
| readme, | ||
| /The Styleguidist demo is a separate view over the same source files\./ | ||
| ); | ||
| assert.match( | ||
| readme, | ||
| /It renders markdown docs and live examples from `playground\/src`, organized by `playground\/styleguide\/styleguide\.config\.js`\./ | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Matching exact sentences from the README is prone to failure upon minor edits. Using partial matches for essential keywords or structural elements (like headers) would be more robust.
test('README gives beginners a reading path and clarifies the styleguide', () => {
assert.match(readme, /### \*\*How to Use This Guide\*\*/);
assert.match(readme, /Start with the \[Playground Project\]/);
assert.match(readme, /### \*\*Understanding the Playground and Style Guide\*\*/);
assert.match(readme, /Styleguidist demo is a separate view/);
assert.match(readme, /organized by `playground\/styleguide\/styleguide\.config\.js`/);
});
Closes #82
Automated fix for bounty issue ($50 USD).
Fix for #82