-
Notifications
You must be signed in to change notification settings - Fork 2
Thro error for no Schema found #1
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdded an explicit return type to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as CLI
participant CFG as cfgUtils.getDirSchemas
participant FS as File System
U->>CLI: Run command
CLI->>CFG: getDirSchemas(dir)
CFG->>FS: Read schema files
FS-->>CFG: File contents
CFG-->>CLI: schemas[]
alt No schemas found
note right of CLI #f9f0c1: Runtime guard added
CLI-->>U: Error("No Schemas found")
CLI-->>CLI: Terminate
else Schemas present
CLI->>CLI: Proceed to sync datasets/tables
CLI-->>U: Completion/next steps
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/cfgUtils.ts (1)
56-56: Add space after colon and improve type formatting.The return type annotation is missing proper spacing and could be more readable.
Apply this diff to improve the type formatting:
-export const getDirSchemas = async (dir: string) :Promise<Array<{name:string,schema:TableSchema}>>=> { +export const getDirSchemas = async (dir: string): Promise<Array<{ name: string; schema: TableSchema }>> => {src/cli.ts (1)
28-30: Fix comparison operator spacing and error constructor consistency.Two minor issues with the guard implementation:
- Missing spaces around the equality operator
- Inconsistent with other error throwing patterns in the file (Line 22 uses
new Error())Apply this diff to improve consistency and formatting:
- if (schemas.length==0){ - throw Error('No Schemas found') + if (schemas.length === 0) { + throw new Error('No Schemas found'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cfgUtils.ts(1 hunks)src/cli.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/cfgUtils.ts (1)
src/bigQueryService.ts (1)
TableSchema(24-27)
SemenchenkoVitaliy
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.
Fix typo in commit message and use Conventional Commits format, e.g. feat: throw error when no schemas are found
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
🧹 Nitpick comments (1)
src/cli.ts (1)
28-30: Consider making the error message more actionable.The error message could be more helpful by including the path that was checked, making it easier for users to diagnose the issue.
Apply this diff to improve the error message:
if (schemas.length === 0) { - throw new Error('No Schemas found'); + throw new Error(`No schema files found in directory: ${config.schemasPath}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cfgUtils.ts(1 hunks)src/cli.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/cfgUtils.ts (1)
src/bigQueryService.ts (1)
TableSchema(24-27)
🔇 Additional comments (1)
src/cfgUtils.ts (1)
56-56: LGTM! Good addition of explicit return type.Adding the explicit return type improves type safety and serves as inline documentation for the function's API contract.
| throw new Error('Credentials must be specified'); | ||
| } | ||
|
|
||
| const bq = await new BigQueryNodeService(credentials); |
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.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the await new pattern.
The await new BigQueryNodeService(credentials) pattern is unusual. Constructors typically aren't async and don't return Promises. If async initialization is needed, verify that this class is intentionally designed to return a Promise from its constructor, or consider using a static factory method instead.
Run the following script to examine the BigQueryNodeService constructor:
Also, as mentioned in past review comments, please run lint to ensure code style compliance:
🏁 Script executed:
#!/bin/bash
# Description: Check the BigQueryNodeService constructor implementation
# Search for the class definition and constructor
ast-grep --pattern $'class BigQueryNodeService {
$$$
constructor($$$) {
$$$
}
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Run lint to check for any style/formatting issues
npm run lintLength of output: 157
🏁 Script executed:
#!/bin/bash
# Locate BigQueryNodeService class definition
rg -nP --type=ts 'class\s+BigQueryNodeService\b' -C3Length of output: 488
🏁 Script executed:
#!/bin/bash
# Check for any constructor definitions in BigQueryNodeService
rg -n 'constructor' -C2 src/bigQueryNodeService.tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Show static async create method in BigQueryNodeService
rg -nP 'static\s+async\s+create' -C10 src/bigQueryNodeService.tsLength of output: 609
🏁 Script executed:
#!/bin/bash
# Display BigQueryService definition and constructor
rg -nP 'export class BigQueryService' -C20 src/bigQueryService.ts
rg -n 'constructor' -C2 src/bigQueryService.tsLength of output: 1829
Use BigQueryNodeService.create() instead of awaiting the constructor
In src/cli.ts:25, replace
const bq = await new BigQueryNodeService(credentials);with
const bq = await BigQueryNodeService.create();so credentials are loaded before instantiation.
🤖 Prompt for AI Agents
In src/cli.ts around line 25, replace the awaited constructor call with the
class factory: remove "await new BigQueryNodeService(credentials)" and call
"await BigQueryNodeService.create()" instead so the service loads credentials
internally before instantiation; ensure any earlier credential-loading code is
kept or removed as appropriate and that the returned instance is assigned to the
same variable name (bq).
SemenchenkoVitaliy
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.
Do not forget to squash commits before they are merged.
Summary by CodeRabbit
New Features
Refactor