Conversation
Implement parser support for relation declarations.\nThe parser now collects relation spans, constructs\nN_RELATION_DECL nodes, and provides a typed Relation\nwrapper with helpers to inspect name, role, columns\nand primary key. Tests cover input, output and internal\nrelations.
Reviewer's GuideImplements relation declaration parsing by extending the parser to collect relation spans, injecting N_RELATION_DECL nodes in the CST, providing a typed Relation AST wrapper with helper methods, and covering input/output/internal relations in new tests. Class diagram for new and updated AST node wrappers (Relation, Root)classDiagram
class Root {
+Vec<Relation> relations()
}
class Relation {
+syntax() SyntaxNode
+name() Option<String>
+is_input() bool
+is_output() bool
+columns() Vec<(String, String)>
+primary_key() Option<Vec<String>>
}
Root --> "*" Relation : contains
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update introduces support for Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Source Code
participant Parser as Parser
participant CST as CST Builder
participant AST as Typed AST
Source->>Parser: parse(src)
Parser->>Parser: Tokenise input
Parser->>Parser: collect_relation_spans()
Parser->>CST: build_green_tree(tokens, imports, typedefs, relations)
CST->>AST: Construct typed nodes (including Relation)
AST->>AST: Provide methods (name, columns, pk, etc.)
AST->>Parser: Return Parsed object with relations
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)`**/*.rs`: Document public APIs using Rustdoc comments (`///`) so documentation ...
📄 Source: CodeRabbit Inference Engine (AGENTS.md) List of files the instruction was applied to:
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
🔇 Additional comments (4)
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 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 (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- The
collect_relation_spansimplementation only grabs until the end of the current line, so multi‐line relation declarations (e.g. wrapping columns on multiple lines) won’t be detected correctly—consider extending span capture to handle declarations spanning multiple lines. handle_inputandhandle_outputincollect_relation_spansare identical; you can factor them into a single helper to reduce duplication.- Parsing
columns()andprimary_key()by string-splitting the raw syntax text is fragile (e.g. comments, nested types, or extra whitespace); consider traversing the syntax node’s children/tokens to build these structures more robustly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `collect_relation_spans` implementation only grabs until the end of the current line, so multi‐line relation declarations (e.g. wrapping columns on multiple lines) won’t be detected correctly—consider extending span capture to handle declarations spanning multiple lines.
- `handle_input` and `handle_output` in `collect_relation_spans` are identical; you can factor them into a single helper to reduce duplication.
- Parsing `columns()` and `primary_key()` by string-splitting the raw syntax text is fragile (e.g. comments, nested types, or extra whitespace); consider traversing the syntax node’s children/tokens to build these structures more robustly.
## Individual Comments
### Comment 1
<location> `src/parser/mod.rs:716` </location>
<code_context>
+ .any(|e| e.kind() == SyntaxKind::K_OUTPUT)
+ }
+
+ /// Columns declared for the relation.
+ #[must_use]
+ pub fn columns(&self) -> Vec<(String, String)> {
+ let text = self.syntax.text().to_string();
+ let inner = text
</code_context>
<issue_to_address>
The columns() method uses string splitting, which may be fragile for complex column types.
String splitting may fail for column types containing commas, colons, or parentheses. Using the syntax tree for parsing would provide more reliable column extraction.
</issue_to_address>
### Comment 2
<location> `src/parser/mod.rs:735` </location>
<code_context>
+ .collect()
+ }
+
+ /// Primary key column names if specified.
+ #[must_use]
+ pub fn primary_key(&self) -> Option<Vec<String>> {
+ let text = self.syntax.text().to_string();
+ let inner = text
</code_context>
<issue_to_address>
primary_key() parsing is brittle and may misbehave with unusual formatting.
Parsing with string splitting is fragile; using the syntax tree to extract primary key columns would be more reliable and handle varied formatting.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/language.rs(1 hunks)src/parser/mod.rs(7 hunks)tests/parser.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.rs`: Document public APIs using Rustdoc comments (`///`) so documentation ...
**/*.rs: Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Userstestfixtures for shared setup.
Replace duplicated tests with#[rstest(...)]parameterised cases.
Prefermockallfor mocks/stubs.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
src/language.rstests/parser.rssrc/parser/mod.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments must use en-GB-oxendict spelling and grammar.
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/language.rstests/parser.rssrc/parser/mod.rs
🧬 Code Graph Analysis (1)
tests/parser.rs (1)
src/parser/mod.rs (8)
parse(93-105)relations(549-555)is_input(702-706)is_output(710-714)name(622-628)name(687-698)columns(718-733)primary_key(737-750)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (3)
src/language.rs (1)
145-145: LGTM!The addition of
N_RELATION_DECLfollows the established naming convention and is appropriately placed with other declaration node types.tests/parser.rs (1)
55-68: Well-structured test coverage!The test fixtures and corresponding tests provide comprehensive coverage for relation parsing, including:
- Different relation types (input, output, internal)
- Primary key variations (single column, compound, none)
- Proper verification of all relation properties
The tests follow the established patterns in the file and use
rstestfixtures appropriately.Also applies to: 299-367
src/parser/mod.rs (1)
737-750: Our sandbox environment can’t be reached without cloning the repo first. To confirm howcolumns()is implemented and whether it also uses string slicing, let’s search for its definition.Could you run this in your local checkout?
rg -n "fn columns" -n src/parser/mod.rsThat will show if
columns()uses similar fragile string parsing.
If it does, we should recommend replacing both methods with AST traversal rather than text splits; otherwise, we only need to updateprimary_key().Let me know what you find.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/parser/mod.rs (1)
775-830: Column parsing implementation is more robust than previous feedback suggested.The current implementation properly traverses the syntax tree and handles nested parentheses with depth tracking, which addresses the concerns raised in past reviews about string splitting. This approach is much more reliable for complex column types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/parser/mod.rs(7 hunks)tests/parser.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.rs`: Document public APIs using Rustdoc comments (`///`) so documentation ...
**/*.rs: Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Userstestfixtures for shared setup.
Replace duplicated tests with#[rstest(...)]parameterised cases.
Prefermockallfor mocks/stubs.
Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
tests/parser.rssrc/parser/mod.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments must use en-GB-oxendict spelling and grammar.
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/parser.rssrc/parser/mod.rs
🧬 Code Graph Analysis (2)
tests/parser.rs (1)
src/parser/mod.rs (14)
parse(93-105)relations(606-612)is_input(759-763)is_output(767-771)name(679-685)name(744-755)columns(775-830)primary_key(834-930)errors(82-84)syntax(554-556)syntax(624-626)syntax(631-645)syntax(673-675)syntax(738-740)
src/parser/mod.rs (3)
src/parser/token_stream.rs (4)
tokens(105-107)src(118-120)peek(76-78)new(41-47)src/parser/span_collector.rs (1)
new(44-50)src/language.rs (1)
kind_to_raw(183-188)
🔇 Additional comments (9)
tests/parser.rs (5)
55-73: Test fixtures are well-structured and comprehensive.The fixtures cover all the essential relation declaration scenarios: input relations with primary keys, output relations without primary keys, internal relations with compound primary keys, and multiline formatting. This provides good test coverage for the new relation parsing functionality.
304-324: Input relation test is thorough and correct.The test properly validates all aspects of the parsed input relation including the input/output flags, name extraction, column parsing, and primary key identification.
326-346: Output relation test correctly validates the parsing.The test appropriately checks that output relations are identified correctly and validates the expected behaviour when no primary key is specified.
348-372: Internal relation test validates compound primary keys well.The test effectively verifies parsing of relations that are neither input nor output, and correctly handles compound primary keys with multiple columns.
374-394: Multiline relation test with round-trip validation is excellent.The test not only verifies correct parsing of multiline declarations but also ensures that the CST preserves the original formatting through the round-trip test. This is crucial for tools that need to maintain source formatting.
src/parser/mod.rs (4)
94-97: Parsing pipeline updates are consistent and well-integrated.The changes to include relation spans in the parsing pipeline follow the established patterns for imports and typedefs. The documentation has been appropriately updated to reflect the new functionality.
Also applies to: 107-127
414-418: CST construction updates follow established patterns.The changes to
build_green_treecorrectly integrate relation spans into the CST construction process, maintaining consistency with how imports and typedefs are handled.Also applies to: 424-454
604-612: Root::relations() method is well-implemented.The method follows the established pattern of other collection methods and provides typed access to relation declarations.
335-340: Rust nightly channel in use – Let chains supportedThe
rust-toolchainfile pins the project tonightly-2025-06-10, which fully supports let chains. No changes required.
1 participant
Summary
N_RELATION_DECLnodesRelationwrappers with helpersTesting
cargo clippy --all-targets --all-features -- -D warningscargo test --all-targets --all-featureshttps://chatgpt.com/codex/tasks/task_e_6860946763f88322b755ad3f05e05179
Summary by Sourcery
Implement parsing of relation declarations and expose them in the CST and AST with helpers.
New Features:
Tests: