-
Notifications
You must be signed in to change notification settings - Fork 7
Move existing generators to a separate file #328
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors code generation for the weak-node-api package by extracting generator functions into separate files, preparing the codebase for additional generators to be added in a future PR.
Key Changes:
- Extracted generator functions from
generate-weak-node-api.tsinto dedicated generator modules - Introduced a shared utility module for common generation logic
- Enhanced error handling for the
clang-formatsubprocess call
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
packages/weak-node-api/scripts/generators/weak-node-api.ts |
New file containing the extracted generateHeader and generateSource generator functions |
packages/weak-node-api/scripts/generators/shared.ts |
New file with shared generateFunction utility for C/C++ function code generation |
packages/weak-node-api/scripts/generate-weak-node-api.ts |
Updated to import and use the extracted generators; improved error handling for clang-format |
| return ` | ||
| #pragma once | ||
| #include <node_api.h> // Node-API |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Corrected comment: 'Node-API' should be 'Node-API headers' for consistency with other include comments.
| #include <node_api.h> // Node-API | |
| #include <node_api.h> // Node-API headers |
| * Generates source code for a version script for the given Node API version. | ||
| */ | ||
| export function generateHeader(functions: FunctionDecl[]) { | ||
| return ` |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The header lacks a #pragma once include guard comment explaining its purpose. The generated header should include a brief description of what this file provides (e.g., 'Weak Node-API host injection interface').
| return ` | |
| return ` | |
| /** | |
| * @file weak_node_api.hpp | |
| * @brief Weak Node-API host injection interface. | |
| * | |
| * This header provides the struct and injection function for | |
| * dynamically supplying Node-API function pointers to the host. | |
| * | |
| * #pragma once is used as an include guard to prevent multiple inclusion. | |
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be adding this in a followup PR.
| const { status, stderr } = cp.spawnSync("clang-format", ["-i", outputPath], { | ||
| encoding: "utf8", | ||
| }); | ||
| assert.equal(status, 0, `Failed to format ${fileName}: ${stderr}`); |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message will display 'undefined' when stderr is empty. The spawnSync options specify encoding: 'utf8', but when status is non-zero and stderr is empty, this will show an unhelpful message. Consider using stderr || 'no error output' or checking if stderr exists before interpolating it.
| assert.equal(status, 0, `Failed to format ${fileName}: ${stderr}`); | |
| assert.equal(status, 0, `Failed to format ${fileName}: ${stderr || "no error output"}`); |
fe4d1f2 to
845f6bb
Compare
As a preparation for another PR adding more generators.