add script to post process docs #4838
Conversation
WalkthroughThe changes introduce a new post-processing step in the documentation generation workflow. This includes an Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (6)
docs/scripts/post-process.js (5)
1-4: LGTM! Consider adding error handling for path resolution.The import statements and path resolution look good. However, it might be beneficial to add error handling for the path resolution to ensure the script fails gracefully if run from an unexpected location.
Consider adding a check like this:
const buildPath = path.resolve(__dirname, "../build"); if (!fs.existsSync(buildPath)) { console.error(`Build directory not found: ${buildPath}`); process.exit(1); }
6-11: LGTM! Consider moving function declaration.The file finding and substitution definition look good and align with the PR objectives. For improved readability, consider moving the
findHTMLFilesfunction declaration before its usage.You could restructure the file like this:
const fs = require("fs"); const path = require("path"); function findHTMLFiles(dir) { // ... (existing implementation) } const buildPath = path.resolve(__dirname, "../build"); const htmlFiles = findHTMLFiles(buildPath); // ... (rest of the code)
13-21: LGTM! Consider adding error handling for file operations.The main processing loop looks good and efficiently applies the substitutions. However, it's important to add error handling for file reading and writing operations to make the script more robust.
Consider wrapping the file operations in try-catch blocks:
for (const file of htmlFiles) { try { let content = fs.readFileSync(file, "utf8"); if (content.includes("__fo_doc_sub")) { for (const [key, value] of Object.entries(substitutions)) { content = content.replace(key, value); } fs.writeFileSync(file, content); console.log(`Processed: ${file}`); } } catch (error) { console.error(`Error processing file ${file}: ${error.message}`); } }
23-39: LGTM! Consider performance and error handling improvements.The
findHTMLFilesfunction correctly implements a recursive search for HTML files. However, there are a few areas where it could be improved:
- For large directory structures, using synchronous file system operations might lead to performance issues.
- There's no error handling for file system operations.
Consider these improvements:
- Use asynchronous file system operations for better performance:
async function findHTMLFiles(dir) { const files = await fs.promises.readdir(dir); const htmlFiles = []; for (const file of files) { const filePath = path.join(dir, file); const stat = await fs.promises.lstat(filePath); if (stat.isDirectory()) { htmlFiles.push(...await findHTMLFiles(filePath)); } else if (file.endsWith(".html")) { htmlFiles.push(filePath); } } return htmlFiles; }
- Add error handling:
async function findHTMLFiles(dir) { try { const files = await fs.promises.readdir(dir); const htmlFiles = []; for (const file of files) { try { const filePath = path.join(dir, file); const stat = await fs.promises.lstat(filePath); if (stat.isDirectory()) { htmlFiles.push(...await findHTMLFiles(filePath)); } else if (file.endsWith(".html")) { htmlFiles.push(filePath); } } catch (error) { console.error(`Error processing ${file}: ${error.message}`); } } return htmlFiles; } catch (error) { console.error(`Error reading directory ${dir}: ${error.message}`); return []; } }Note that if you implement these changes, you'll need to update the main script to use
async/awaitas well.
1-39: Overall assessment: Good implementation with room for improvement.The script successfully implements the post-processing of HTML files to add "NEW" badges as described in the PR objectives. It's a straightforward and functional solution. However, there are several areas where it could be improved:
- Error handling: Add checks for directory existence and wrap file operations in try-catch blocks.
- Performance: Consider using asynchronous file system operations, especially for the
findHTMLFilesfunction.- Code structure: Move function declarations before their usage for better readability.
- Logging: Add more verbose logging to track the script's progress and any issues encountered.
These improvements would make the script more robust, efficient, and maintainable.
docs/generate_docs.bash (1)
136-138: LGTM! Consider adding error handling and documentation.The addition of the post-processing step aligns well with the PR objectives. Here are some suggestions to improve robustness and clarity:
Add error handling for the Node.js script execution. This will help catch and report any issues that might occur during post-processing.
Include a brief comment explaining the purpose of the post-processing step. This will help future maintainers understand the script's functionality.
Consider updating the code as follows:
echo "Post-processing docs" -node ./scripts/post-process.js +# Post-process the docs to add "NEW" labels +if ! node ./scripts/post-process.js; then + echo "Error: Post-processing failed" >&2 + exit 1 +fiThis change adds error handling and a comment explaining the purpose of the post-processing step.
What changes are proposed in this pull request?
Add a script to post-process docs. Currently support adding bolded
NEWlabel in orange as shown below:docs/source/index.rst:
__SUB_NEW__in snippet above will be replaced with HTMLHow is this patch tested? If it is not, please explain why.
Using docs build
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation