fix(cli): add support for custom proto lock file paths in grpc generate command #2008
Conversation
WalkthroughThe command-line interface for generating protobuf schemas now supports an optional Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
clayne11
force-pushed
the
curtis.layne/proto-lock-fix
branch
from
2db1f78 to
caeb35c
Compare
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/src/commands/grpc-service/commands/generate.ts(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cli/src/commands/grpc-service/commands/generate.ts (1)
cli/src/commands/router/commands/plugin/toolchain.ts (1)
generateProtoAndMapping(354-393)
🪛 GitHub Actions: wgc CI
cli/src/commands/grpc-service/commands/generate.ts
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
cli/src/commands/grpc-service/commands/generate.ts (5)
1-1: LGTM: Import addition is appropriate.The addition of the
existsimport fromnode:fs/promisesis correct and necessary for the async file existence checks in the newfetchLockDatafunction.
19-22: LGTM: CLI option implementation is well-structured.The new
--proto-lockoption is properly implemented with both short (-l) and long form, includes appropriate description, and follows the existing pattern for optional arguments.
57-57: LGTM: Parameter passing is correct.The
options.protoLockparameter is correctly passed to thegenerateProtoAndMappingfunction.
87-87: LGTM: Function signature update is appropriate.The addition of the optional
protoLockFileparameter maintains backward compatibility while enabling the new functionality.
96-96: LGTM: Refactoring to async helper function improves code organization.The replacement of inline synchronous lock data reading with the async
fetchLockDatahelper function improves code organization and enables the new custom proto lock file functionality.
clayne11
force-pushed
the
curtis.layne/proto-lock-fix
branch
from
caeb35c to
a6949bd
Compare
|
Hi @clayne11, thanks for the fix! We're currently experimenting with coderabbitai pull request AI review thing you can see above, which likes to make a lot of noise while PRs are being worked on if they aren't in the draft state. Until CI (looks like just a prettier fail) is fixed and you're happy with the state, would you mind marking it as a draft? |
clayne11
force-pushed
the
curtis.layne/proto-lock-fix
branch
2 times, most recently
from
57bb4f6 to
93b4db0
Compare
|
Should be good to go now @endigma. CodeRabbit was helpful, it pointed out a real issue. |
clayne11
force-pushed
the
curtis.layne/proto-lock-fix
branch
2 times, most recently
from
c78fa0a to
54a0385
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/src/commands/grpc-service/commands/generate.ts(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cli/src/commands/grpc-service/commands/generate.ts (2)
cli/src/commands/router/commands/plugin/toolchain.ts (1)
generateProtoAndMapping(354-393)protographic/src/index.ts (2)
compileGraphQLToMapping(15-31)compileGraphQLToProto(52-78)
🪛 GitHub Check: build_test_node_matrix (22.x)
cli/src/commands/grpc-service/commands/generate.ts
[failure] 5-5:
Ora not found in 'ora'
[failure] 4-4:
node:fs/promises import should occur before import of @wundergraph/protographic
🪛 GitHub Check: build_test_node_matrix (20.x)
cli/src/commands/grpc-service/commands/generate.ts
[failure] 5-5:
Ora not found in 'ora'
[failure] 4-4:
node:fs/promises import should occur before import of @wundergraph/protographic
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
cli/src/commands/grpc-service/commands/generate.ts (9)
10-16: Well-defined CLI options type.The
CLIOptionstype is well-structured and clearly defines the expected command-line options with appropriate optional properties.
26-30: Excellent implementation of the new proto-lock option.The new
--proto-lockoption is well-documented with clear help text explaining its purpose and default behavior. This addresses the PR objective of supporting custom proto lock file paths for build environments like Bazel.
42-42: Improved function signature with proper typing.The function signature has been improved to use the properly typed
CLIOptionsinstead ofany, which enhances type safety and code maintainability.
53-63: Good conversion to async file operations.The conversion from synchronous
existsSyncandlstatSyncto the new asyncexistsfunction is a good improvement that makes the code more consistent with the async/await pattern used throughout the file.
65-73: Clean refactoring to options object pattern.The refactoring from multiple parameters to a single options object improves the function's API and makes it easier to extend in the future. The explicit parameter names make the code more readable.
94-102: Well-structured generation options type.The
GenerationOptionstype is comprehensive and properly typed, including theOratype for the spinner parameter.
107-115: Improved function signature with default parameter.The destructured parameter approach with a default value for
lockFileis clean and provides a good fallback behavior when no custom lock file is specified.
138-147: Simple and effective lock data fetching.The
fetchLockDatafunction is well-implemented with proper error handling. It checks for file existence before attempting to read and parse the JSON, returningundefinedgracefully if the file doesn't exist or is invalid.
149-157: Proper async replacement for existsSync.The
existsfunction is a good async replacement forexistsSyncusing the recommendedaccessapproach. The error handling is appropriate, returningfalsewhen the file doesn't exist or isn't accessible.
clayne11
force-pushed
the
curtis.layne/proto-lock-fix
branch
from
50e23fe to
fb29aa3
Compare
clayne11
force-pushed
the
curtis.layne/proto-lock-fix
branch
2 times, most recently
from
35db936 to
e46f660
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/src/commands/grpc-service/commands/generate.ts (1)
138-145: Fix the negated condition to follow linting rules.The static analysis correctly identifies an issue with the negated condition. The logic can be simplified and made more readable.
Apply this diff to fix the linting issue:
-async function fetchLockData(lockFile: string): Promise<ProtoLock | undefined> { - if (!(await exists(lockFile))) { - return undefined; - } - - const existingLockData = JSON.parse(await readFile(lockFile, 'utf8')); - return existingLockData != null ? existingLockData : undefined; -} +async function fetchLockData(lockFile: string): Promise<ProtoLock | undefined> { + if (await exists(lockFile)) { + const existingLockData = JSON.parse(await readFile(lockFile, 'utf8')); + return existingLockData || undefined; + } + + return undefined; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/src/commands/grpc-service/commands/generate.ts(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cli/src/commands/grpc-service/commands/generate.ts (2)
cli/src/commands/router/commands/plugin/toolchain.ts (1)
generateProtoAndMapping(354-393)protographic/src/index.ts (2)
compileGraphQLToMapping(15-31)compileGraphQLToProto(52-78)
🪛 GitHub Check: build_test_node_matrix (22.x)
cli/src/commands/grpc-service/commands/generate.ts
[failure] 144-144:
Unexpected negated condition
🪛 GitHub Check: build_test_node_matrix (20.x)
cli/src/commands/grpc-service/commands/generate.ts
[failure] 144-144:
Unexpected negated condition
🪛 GitHub Actions: wgc CI
cli/src/commands/grpc-service/commands/generate.ts
[error] 144-144: ESLint: Unexpected negated condition (unicorn/no-negated-condition)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
cli/src/commands/grpc-service/commands/generate.ts (10)
1-6: LGTM! Import organization follows conventions.The imports are now properly organized with Node.js built-ins first, then third-party packages, and the type import for
Orais correctly specified. This addresses the previous static analysis issues.
10-16: Well-defined CLI options type.The
CLIOptionstype clearly defines all the command-line options including the newprotoLockfield. The optional fields are appropriately marked.
26-30: Excellent addition of the --proto-lock option.The new option provides clear documentation and addresses the core requirement for supporting custom proto lock file paths in build environments like Bazel. The description clearly explains the default behavior.
42-42: Good refactoring to use typed options.The function signature now uses the well-defined
CLIOptionstype instead ofany, improving type safety.
53-63: Proper conversion to async filesystem operations.The file existence and directory checks now use the async
existsfunction andlstat, which is more appropriate for CLI operations that shouldn't block the event loop.
65-73: Clean refactoring to options object.The function call now uses a clear options object instead of multiple positional parameters, making it more maintainable. The new
lockFileparameter correctly passes the custom proto lock path.
94-102: Well-structured generation options type.The
GenerationOptionstype provides clear documentation of all parameters needed for proto generation, including the new optionallockFileparameter.
107-115: Excellent function signature improvement.The destructured parameters with default value for
lockFilemake the function more flexible and maintainable. The default fallback path maintains backward compatibility.
122-122: Simplified lock data loading.The extraction of lock data fetching into a separate function improves code organization and readability.
147-155: Well-implemented async exists helper.The function properly uses
accesswithconstants.R_OKas recommended, and correctly handles the promise rejection to return a boolean. The comment explaining whyexistsfromnode:fsis not recommended is helpful.
clayne11
force-pushed
the
curtis.layne/proto-lock-fix
branch
2 times, most recently
from
cf11922 to
7ae4b1a
Compare
endigma
left a comment
endigma
left a comment
There was a problem hiding this comment.
LGTM, thanks for the cleanup of option types as well!
…te command - Add --proto-lock/-l option to specify existing proto lock file path. This is necessary for environments like Bazel where input and output files must be at different paths. Making assumptions about paths in general with Bazel often leads to issues due to Bazel's sandboxing.
clayne11
force-pushed
the
curtis.layne/proto-lock-fix
branch
from
7ae4b1a to
3464a4e
Compare
4 participants
Add
--proto-lock/-loption to specify existing proto lock file path. This is necessary for environments like Bazel where input and output files must be at different paths. Making assumptions about paths in general with Bazel often leads to issues due to Bazel's sandboxing.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Checklist