-
Notifications
You must be signed in to change notification settings - Fork 11
indexer: rename pgTable to pgIndexerTable #110
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
📝 WalkthroughWalkthroughThe pull request introduces a new JSON file for the Changes
Possibly related PRs
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)
- change/@apibara-indexer-43e28d2b-5062-4907-8504-f0287b439697.json (1 hunks)
- examples/starknet-indexer/src/indexer.ts (1 hunks)
- packages/indexer/src/sinks/drizzle/drizzle.test.ts (1 hunks)
- packages/indexer/src/sinks/drizzle/utils.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- change/@apibara-indexer-43e28d2b-5062-4907-8504-f0287b439697.json
🧰 Additional context used
📓 Learnings (1)
packages/indexer/src/sinks/drizzle/drizzle.test.ts (1)
Learnt from: fracek
PR: apibara/typescript-sdk#109
File: packages/indexer/src/sinks/drizzle/drizzle.test.ts:16-18
Timestamp: 2024-10-22T11:16:00.689Z
Learning: In `packages/indexer/src/sinks/drizzle`, the import references are correct and up-to-date according to the new code structure.
🔇 Additional comments (5)
examples/starknet-indexer/src/indexer.ts (2)
4-4: LGTM: Clean import rename
The import statement has been correctly updated to use pgIndexerTable.
12-12: LGTM: Table definition updated consistently
The table definition has been properly updated to use pgIndexerTable while maintaining the existing table structure and configuration.
packages/indexer/src/sinks/drizzle/utils.ts (1)
Line range hint 32-68: LGTM: Type renaming is clean and maintains compatibility.
The renaming from PgTableWithCursorFn to PgIndexerTableWithCursorFn is well-structured and preserves the existing type constraints and cursor column requirements.
packages/indexer/src/sinks/drizzle/drizzle.test.ts (2)
18-18: LGTM: Import statement updated correctly.
The import statement has been properly updated to use pgIndexerTable instead of pgTable, which aligns with the PR objectives.
20-23: LGTM: Table definition updated correctly.
The table definition has been properly migrated to use pgIndexerTable while maintaining the same schema structure. This change allows for the coexistence of both pgTable and pgIndexerTable in schema files as intended.
| export const pgIndexerTable: PgIndexerTableWithCursorFn = ( | ||
| name, | ||
| columns, | ||
| extraConfig?, | ||
| ) => { |
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.
💡 Codebase verification
Found remaining usage of pgTable in examples that needs updating
The renaming from pgTable to pgIndexerTable is incomplete. While the implementation file correctly aliases the import from drizzle-orm, there's still direct usage of pgTable in:
examples/indexer/src/indexer.ts: Uses the originalpgTablefrom drizzle-orm directly
This example should be updated to use the new pgIndexerTable for consistency with the rest of the codebase.
🔗 Analysis chain
LGTM: Function renaming is consistent with type changes.
The implementation maintains the same functionality while adopting the new naming convention.
Let's verify there are no remaining references to the old name:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of 'pgTable' that might need updating
rg 'pgTable' --type ts --type tsx --type js --type jsx -g '!*.test.*' -g '!*.spec.*'
Length of output: 112
Script:
#!/bin/bash
# Search for any remaining instances of 'pgTable' using ripgrep with correct file type options
rg 'pgTable' --type typescript --type javascript -g '!*.test.*' -g '!*.spec.*'
Length of output: 113
Script:
#!/bin/bash
# Search for any remaining instances of 'pgTable' using basic ripgrep without file type filters
# but exclude test files using glob patterns
rg 'pgTable' -g '!*.test.*' -g '!*.spec.*' -g '*.ts' -g '*.js'
Length of output: 451
jaipaljadeja
left a comment
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.
Looks good. Thank you!
This enables developers to use plain pgTable and pgIndexerTable in the same schema file.