-
Notifications
You must be signed in to change notification settings - Fork 1
comments and spans #19
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ import type { Dialect, DialectName } from './dialects.js'; | |||||
| import { dialectFromString, GenericDialect } from './dialects.js'; | ||||||
| import { ParserError } from './types/errors.js'; | ||||||
| import type { Statement } from './types/ast.js'; | ||||||
| import type { SourceComment, ParseWithCommentsResult } from './types/comments.js'; | ||||||
|
||||||
| import type { SourceComment, ParseWithCommentsResult } from './types/comments.js'; | |
| import type { ParseWithCommentsResult } from './types/comments.js'; |
Copilot
AI
Mar 20, 2026
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.
parseWithComments() ignores this.options (e.g., recursionLimit/trailingCommas) even when the caller configured them via withOptions() / withRecursionLimit(), unlike parse(). This is a behavioral inconsistency that can surprise users. Either plumb options through to the WASM API (e.g., add a parse_sql_with_comments_with_options entrypoint) or make this a static-only helper / explicitly reject or document non-empty options.
Copilot
AI
Mar 20, 2026
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.
New public API (parseWithComments) is introduced without corresponding tests. There is existing Vitest coverage for parse/format/validate; add tests that exercise comment extraction and span fields to prevent regressions (including build-export checks if applicable).
| export function parseWithComments(sql: string, dialect: DialectInput = 'generic'): ParseWithCommentsResult<Statement> { | |
| function parseWithComments(sql: string, dialect: DialectInput = 'generic'): ParseWithCommentsResult<Statement> { |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| /** A source code comment extracted from parsed SQL */ | ||
| export interface SourceComment { | ||
| /** "singleLine" for -- comments, "multiLine" for block comments */ | ||
| commentType: 'singleLine' | 'multiLine' | ||
| /** The comment text content (excluding markers) */ | ||
| content: string | ||
| /** For single-line comments, the prefix (e.g. "--", "#") */ | ||
| prefix?: string | ||
| /** Start line (1-based) */ | ||
| startLine: number | ||
| /** Start column (1-based) */ | ||
| startColumn: number | ||
| /** End line (1-based) */ | ||
| endLine: number | ||
| /** End column (1-based) */ | ||
| endColumn: number | ||
| } | ||
|
|
||
| /** Result of parsing SQL with comments */ | ||
| export interface ParseWithCommentsResult<T> { | ||
| statements: T[] | ||
| comments: SourceComment[] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| export * from './ast.js'; | ||
| export * from './errors.js'; | ||
| export * from './comments.js'; |
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.
js_sys::Reflect::set(...).unwrap()can panic in WASM, aborting the module on what should be a recoverable error path. Prefer propagating theJsValueerror (e.g., mapReflect::seterrors into theResult) instead of unwrapping.