feat(napi): Expose node id via SgNode#1629
feat(napi): Expose node id via SgNode#1629HerringtonDarkholme merged 2 commits intoast-grep:mainfrom
Conversation
WalkthroughThe changes introduce a new method, Changes
Poem
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 (
|
|
Can you add a test case for this? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/napi/src/sg_node.rs (1)
165-169: LGTM! Consider enhancing the documentation.The implementation looks good and aligns with the PR objective of exposing node IDs for comparison. Consider enhancing the documentation to explain the purpose and usage:
- /// Returns the node's id + /// Returns the node's unique identifier. + /// This can be used to efficiently compare two nodes for identity.crates/napi/index.d.ts (1)
130-131: Consider enhancing the method documentation.The current documentation "Returns the node's id" could be more descriptive about the method's intended use case for node comparison and any guarantees about id uniqueness.
Consider updating the JSDoc comment to:
- /** Returns the node's id */ + /** + * Returns a unique identifier for the node. + * Can be used to efficiently compare two nodes for identity. + * @returns A unique numeric identifier + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
crates/napi/index.d.ts(1 hunks)crates/napi/src/sg_node.rs(1 hunks)
🔇 Additional comments (2)
crates/napi/src/sg_node.rs (1)
168-168: Verify the safety of usize to u32 casting
The cast from usize to u32 could potentially truncate on 64-bit systems. While node IDs are typically small sequential numbers, we should verify this assumption.
✅ Verification successful
Based on the search results, I can now generate the final response:
The usize to u32 cast for node_id is safe
The node_id implementation in crates/core/src/node.rs shows that IDs are derived from the tree-sitter node IDs, which are sequential and start from 0. Tree-sitter's implementation ensures node IDs are well within u32 range as they represent positions in a single syntax tree. The cast is safe for all practical purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for node_id implementation to verify the ID generation logic
# and ensure it can't exceed u32::MAX
# Look for node_id implementation
ast-grep --pattern 'fn node_id($$$) { $$$}'
# Look for places where node IDs are generated/assigned
rg -A 5 'node_id'
Length of output: 5114
crates/napi/index.d.ts (1)
130-131: LGTM! Implementation aligns with PR objectives.
The addition of the id() method to SgNode class with a number return type correctly maps to the Rust implementation, enabling node comparison functionality as intended.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1629 +/- ##
=======================================
Coverage 87.03% 87.03%
=======================================
Files 95 95
Lines 15277 15277
=======================================
Hits 13296 13296
Misses 1981 1981 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/napi/__test__/index.spec.ts (1)
222-227: Consider adding test cases for negative scenarios.While the current test verifies that the same node returns the same ID, it would be valuable to add test cases that verify:
- Different nodes return different IDs
- Node IDs remain stable across multiple parses of the same code
- Edge cases like empty nodes or invalid patterns
Example addition:
test('get node id', async t => { const sg = parse('console.log(123)') const nodeWithPattern = sg.root().find('console.log($$$)') const nodeWithKind = sg.root().find(kind('call_expression')) t.is(nodeWithPattern!.id(), nodeWithKind!.id()) + // Verify different nodes have different IDs + const differentNode = sg.root().find('console') + t.not(nodeWithPattern!.id(), differentNode!.id()) + + // Verify ID stability across parses + const sg2 = parse('console.log(123)') + const sameNodeDifferentParse = sg2.root().find('console.log($$$)') + t.is(nodeWithPattern!.id(), sameNodeDifferentParse!.id()) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
crates/napi/__test__/index.spec.ts(2 hunks)
🔇 Additional comments (1)
crates/napi/__test__/index.spec.ts (1)
222-227: LGTM! The test case effectively verifies node ID comparison.
The test successfully validates that the same node can be identified consistently using different query methods.
Added one test. But LMK if more tests are needed. I could add one to check for different ids for different nodes. |
|
thanks |
This PR exposes node_id via
SgNode. It's useful to check whether the results of two separatefindoperations are identical when using the NAPI API.Summary by CodeRabbit
id()for theSgNodeclass, allowing users to retrieve the unique identifier of each node.