-
Notifications
You must be signed in to change notification settings - Fork 0
fix: add composite type support #46
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@fhur has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 50 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes involve updates to the base64-encoded strings for navigation and search data in specific JavaScript files. These modifications may alter the datasets used for navigation and search functionalities within the application. Additionally, there is an enhancement to console output messages in the CLI and improvements to progress reporting during schema extraction. The overall structure and logic of the code remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Navigation
participant Search
participant SchemaGenerator
User->>CLI: Initiate connection
CLI-->>User: Display connection message with emoji
User->>Navigation: Request navigation data
Navigation-->>User: Provide updated navigation data
User->>Search: Perform search query
Search-->>User: Provide updated search data
User->>SchemaGenerator: Start schema extraction
SchemaGenerator-->>User: Show progress percentage
SchemaGenerator-->>User: Extraction complete
Poem
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
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/cli/src/cli.ts (1 hunks)
- packages/docs/static/reference/assets/navigation.js (1 hunks)
- packages/docs/static/reference/assets/search.js (1 hunks)
- packages/introspect/src/generate.ts (4 hunks)
Files skipped from review due to trivial changes (1)
- packages/cli/src/cli.ts
Additional comments not posted (5)
packages/docs/static/reference/assets/navigation.js (1)
1-1: Verify compatibility with the updated navigation data format.The base64-encoded string assigned to
window.navigationDatahas been updated, which may indicate a change in the underlying navigation data format.Please ensure that the decoding and processing logic in other parts of the codebase are updated accordingly to handle the new navigation data format correctly. Consider running the following script to search for relevant code snippets:
Verify that any code snippets found are compatible with the updated navigation data format to prevent potential issues with the navigation functionality.
packages/docs/static/reference/assets/search.js (1)
1-1: Base64-encoded search data has been updated. Please provide more context.The base64-encoded string assigned to
window.searchDatahas been modified. To facilitate the review process, please provide insights into the following aspects:
- What is the nature of the change made to the encoded data? Does it represent an update to the existing dataset or a completely new dataset?
- How does this change impact the search functionality? Are there any changes to the search query processing or the returned results?
- Consider adding a comment to document the purpose and version of the encoded data. This will help maintain clarity and make future modifications easier to understand.
packages/introspect/src/generate.ts (3)
Line range hint
73-100: Enhancements to progress reporting look good!The changes introduce a more detailed progress indicator that visually represents the extraction progress as a percentage. The code correctly calculates and reports the progress using
stderr, which is appropriate for this purpose.
209-209: Custom transformer support looks good!The change allows custom transformers to modify/extend the default generation data for applicable table columns. The use of optional chaining ensures safe access to the
transformmethod.
Line range hint
1-500: Remaining changes look good!The remaining code changes are minor and do not require specific comments. They appear to be safe and do not affect the core logic of the file.
| const isComposite = column.type.kind === 'composite'; | ||
| // TODO(fhur): for now, when a type is composite use the "unknown" type. | ||
| // In the future we should add support for composite types. | ||
| const type = isComposite | ||
| ? {} | ||
| : { $ref: `#/$defs/${createTypeDefId(column.type)}` }; |
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.
Composite type handling looks good for now!
The changes introduce a check for composite types and use an empty object as a placeholder, indicating that further support is planned. This is a reasonable temporary solution.
Consider creating a GitHub issue to track the remaining work needed to fully support composite types. Do you want me to open an issue for this?
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.
sure
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.
@fhur, the issue has been created: #47
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue to track adding full support for composite types
gh issue create \
--title "Add support for composite types in schema generation" \
--body $'This issue tracks the work needed to fully support composite types in the schema generation process.\n\nCurrently, composite types are handled as empty objects as placeholders.\n\nReferenced in PR #46: https://github.com/synthql/SynthQL/pull/46\n\nComment: https://github.com/synthql/SynthQL/pull/46#discussion_r1758436671\n\ncc @fhur'
Length of output: 461
SynthQL pull request template
Why
The CLI was breaking when a schema had a composite type. This MR fixes it.
Summary by CodeRabbit