Conversation
Reviewer's GuideThis PR centralizes AST node access by introducing the Class diagram for AstNode trait and macro-based implementationsclassDiagram
class AstNode {
+syntax() : &SyntaxNode<DdlogLanguage>
}
class Import {
+syntax: SyntaxNode<DdlogLanguage>
+... // other methods
}
class TypeDef {
+syntax: SyntaxNode<DdlogLanguage>
+... // other methods
}
class Relation {
+syntax: SyntaxNode<DdlogLanguage>
+... // other methods
}
class Index {
+syntax: SyntaxNode<DdlogLanguage>
+... // other methods
}
class Rule {
+syntax: SyntaxNode<DdlogLanguage>
+... // other methods
}
class Function {
+syntax: SyntaxNode<DdlogLanguage>
+... // other methods
}
class Transformer {
+syntax: SyntaxNode<DdlogLanguage>
+... // other methods
}
AstNode <|.. Import
AstNode <|.. TypeDef
AstNode <|.. Relation
AstNode <|.. Index
AstNode <|.. Rule
AstNode <|.. Function
AstNode <|.. Transformer
note for AstNode "Trait introduced to centralize syntax node access. Implemented via macro for each AST wrapper type."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughUpdate the Changes
Sequence Diagram(s)sequenceDiagram
participant TestModule as tests::parser
participant Parser as crate::parser
participant AST as crate::parser::ast
TestModule->>Parser: Use parser functionality
Parser->>AST: Access AstNode trait (now pub(crate))
AST-->>Parser: Provide AST node logic (internal)
TestModule->>AST: Access AstNode for tests (permitted by crate visibility)
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (1)**/*.rsInstructions used from: Sources: 🔇 Additional comments (5)
✨ 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 and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/parser/mod.rs:404` </location>
<code_context>
+ /// let import = parsed.root().imports().first().unwrap();
+ /// assert_eq!(import.syntax().kind(), SyntaxKind::N_IMPORT_STMT);
+ /// ```
+ #[cfg_attr(not(test), expect(dead_code, reason = "only used in tests"))]
+ // This trait is used exclusively in tests to inspect AST nodes.
+ pub(crate) trait AstNode {
</code_context>
<issue_to_address>
The use of `expect(dead_code)` here may be misleading.
Since `AstNode` is used in the main codebase and not just in tests, the `expect(dead_code)` attribute with this reason may cause confusion. Please clarify the attribute's purpose or remove it if the trait is used outside of tests.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
#[cfg_attr(not(test), expect(dead_code, reason = "only used in tests"))]
// This trait is used exclusively in tests to inspect AST nodes.
pub(crate) trait AstNode {
=======
// This trait is used to inspect AST nodes and is utilized in both the main codebase and tests.
pub(crate) trait AstNode {
>>>>>>> REPLACE
</suggested_fix>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
♻️ Duplicate comments (1)
src/parser/mod.rs (1)
404-407: Clarify theexpect(dead_code)attribute reasoning.The attribute reason states the trait is "primarily exercised through test modules", but since
AstNodeis implemented across multiple AST wrapper types and appears to be part of the public interface, this reasoning may be misleading.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.markdownlint-cli2.jsonc(1 hunks)src/parser/ast/parse_utils.rs(1 hunks)src/parser/mod.rs(8 hunks)src/parser/tests/parser.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🔇 Additional comments (5)
src/parser/mod.rs (3)
415-423: Excellent macro design for eliminating code duplication.The macro effectively unifies the
AstNodetrait implementation across all AST wrapper types, replacing repetitive manual implementations.
662-662: Clean refactoring using the macro consistently across all AST types.The consistent application of
impl_ast_node!across all AST wrapper types successfully eliminates code duplication whilst providing a unified interface.Also applies to: 708-708, 960-960, 1083-1083, 1177-1177, 1285-1285, 1345-1345
1350-1350: Appropriate test module organisation.Adding the parser test module supports the updated trait interface testing.
src/parser/ast/parse_utils.rs (1)
664-664: Necessary import for updated test interface.The
AstNodetrait import correctly supports the refactored test code that uses the unified trait interface.src/parser/tests/parser.rs (1)
7-11: Imports updated correctlySwitching to
crate::…keeps the tests in-crate and removes the external dependency. No further action required.
leynos
force-pushed
the
codex/add-private-trait-astnode-to-parser
branch
from
54c3eda to
181e7a0
Compare
1 participant
Summary
AstNodewith documentationAstNodeimpls viaimpl_ast_node!macrohttps://chatgpt.com/codex/tasks/task_e_68782d2339148322a35ac82c9a5cf93d
Summary by Sourcery
Introduce AstNode trait and impl_ast_node! macro to unify and simplify AST wrapper implementations, remove redundant syntax() methods, relax markdownlint rules for documentation, and update tests to use the new trait and correct language reference.
New Features:
Enhancements:
Tests: