add yaml language support #73
Conversation
|
Warning Rate limit exceeded@anoblet has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
WalkthroughThis change introduces a new language integration package, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LangYaml as @ast-grep/lang-yaml
participant SetupLang as @ast-grep/setup-lang
participant AstGrepNAPI as ast-grep N-API
User->>LangYaml: Install package
LangYaml->>SetupLang: Run postinstall script
SetupLang-->>LangYaml: Setup language integration
User->>LangYaml: Import registration config
User->>AstGrepNAPI: Register YAML language dynamically
AstGrepNAPI-->>User: Language registered
User->>AstGrepNAPI: Parse YAML code
AstGrepNAPI-->>User: Return syntax tree
Possibly related PRs
Suggested reviewers
Poem
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. 🪧 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 (
|
![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
🧹 Nitpick comments (9)
packages/yaml/.gitignore (1)
27-28: Extend ignore patterns for parser binaries.
Consider ignoring platform-specific parser binaries beyondparser.so, such as.dylib,.dll, or.nodefiles, to prevent accidentally checking them into Git. For example:parser.so +parser.dylib +parser.dll +parser.node type.d.tspackages/yaml/README.md (1)
1-2: Capitalize the title consistently.
Consider updating the header to something like# AST‑Grep N‑API Language for YAMLto follow standard capitalization and branding.packages/yaml/index.js (2)
1-3: Consider usingrequire('path')for broader compatibility.
Usingrequire('node:path')works in modern Node.js, butrequire('path')is more universally supported across older Node.js versions:-const path = require('node:path') +const path = require('path')
6-6: Add spacing in theextensionsarray.
For readability, include a space after the comma:- extensions: ["yaml","yml"], + extensions: ["yaml", "yml"],packages/yaml/index.d.ts (1)
1-7: Add JSDoc for clarity and maintainability.Consider documenting each property in the
LanguageRegistrationtype with JSDoc comments to make its purpose explicit for consumers of the typings.packages/yaml/.github/workflows/release.yml (1)
6-13: Consider providing a descriptive default fordry-run.Currently
dry-rundefaults totrue, which is safe—but you may want to document in the README how to override it for real publishes.packages/yaml/package.json (3)
2-4: Populatedescriptionandrepositoryfields.An empty description and missing
repositoryentry reduce discoverability and make automated tooling less informative.
6-12: Expand scripts to include linting or validation.Adding a
lintorvalidatescript (e.g.,npm run lintornpm run test:ci) can catch syntax and style issues before publish.
25-32: Broaden peerDependency version range.Pinning
tree-sitter-clito exactly0.24.6may prevent compatibility with patch releases. Consider using^0.24.6instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
packages/yaml/.github/workflows/release.yml(1 hunks)packages/yaml/.gitignore(1 hunks)packages/yaml/README.md(1 hunks)packages/yaml/index.d.ts(1 hunks)packages/yaml/index.js(1 hunks)packages/yaml/nursery.js(1 hunks)packages/yaml/package.json(1 hunks)packages/yaml/postinstall.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/yaml/nursery.js (2)
packages/yaml/postinstall.js (1)
require(1-1)scripts/nursery/index.ts (1)
setup(38-46)
🔇 Additional comments (12)
packages/yaml/.gitignore (1)
29-30: Verify exclusion of thesrcdirectory.
Ignoringsrcmay inadvertently hide source files if asrcfolder is part of your package. Ensure that any grammar or code generated intosrcis intentionally untracked, or adjust this rule if you need to version-control source code insrc.packages/yaml/postinstall.js (1)
1-4: Postinstall script setup is correct.
Thepostinstallimport and invocation withdirname: __dirnamecorrectly hooks into the package lifecycle to bootstrap the parser.packages/yaml/nursery.js (2)
1-2: Import the nursery setup API.
Destructuringsetupfrom@ast-grep/nurseryand importing the local registration are correct and follow the established pattern.
4-8: Validate nursery configuration.
The configuration properties (dirname,name,treeSitterPackage, andlanguageRegistration) align with other language packages. Ensure that'yaml'matches the expected language identifier throughout the ecosystem.packages/yaml/README.md (2)
5-12: Installation instructions are clear.
The PNPM commands cover all dependencies, including the optional Tree‑sitter CLI.
16-24: Usage example is accurate.
Importing, registering the language, and parsing a YAML string is demonstrated clearly.packages/yaml/index.js (1)
4-9: Exported language registration object is well-structured.
ThelibraryPath,extensions,languageSymbol, andexpandoCharproperties align with otherast-greplanguage packages and will integrate seamlessly.packages/yaml/index.d.ts (1)
9-10: Verify the runtime shape matches the declaration.Ensure that the default export in
index.jsconforms exactly to this interface (has all required fields and optional fields are handled).packages/yaml/.github/workflows/release.yml (2)
1-5: Approve CI trigger configuration.The manual
workflow_dispatchis correctly set up for per‐package publishing.
16-22: Ensure the composite workflow reference is up to date.Verify that
ast-grep/langs/.github/workflows/release.yml@mainreflects the latest release steps and that the repo’smainbranch is stable for reuse in dependent workflows.packages/yaml/package.json (2)
27-29: Lock down critical dependencies.Verify that
@ast-grep/[email protected]has no known vulnerabilities and that this exact version is intended.
43-52: Validate pnpm-onlyBuiltDependencies scope.Confirm that restricting built dependencies to this package and
tree-sitter-clialigns with your monorepo’s install strategy and doesn’t inadvertently exclude needed packages.
anoblet
force-pushed
the
yaml
branch
2 times, most recently
from
a17904f to
1a04f81
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/yaml/nursery.js (1)
10-50: 🛠️ Refactor suggestionTest coverage should be expanded to include more YAML features.
The current test suite provides basic verification of YAML parsing and pattern matching, which is a good start. However, to ensure robust YAML support, consider adding tests for more YAML features:
- Multi-document YAML (separated by
---)- Anchors and aliases (
&anchor,*alias)- Quoted strings and multi-line strings
- Flow-style collections (
[inline, arrays],{object: notation})- Comments and how they're handled in the AST
- Error handling for malformed YAML
This aligns with the previous review request to add tests referencing the Rust implementation at
crates/language/src/yaml.rs.Here's a sample extension for testing flow-style collections:
// Add after line 49 // Test flow-style collections const flowStyle = ` flows: array: [1, 2, 3] object: {key1: value1, key2: value2} ` const sg5 = parse(flowStyle) const root5 = sg5.root() const arrayNode = root5.find('array: $ARRAY') assert.equal(arrayNode.kind(), 'block_mapping_pair') assert.equal(arrayNode.find('$ARRAY').kind(), 'flow_sequence') const objectNode = root5.find('object: $OBJECT') assert.equal(objectNode.kind(), 'block_mapping_pair') assert.equal(objectNode.find('$OBJECT').kind(), 'flow_mapping')
🧹 Nitpick comments (2)
packages/yaml/nursery.js (2)
28-37: Nested structure test looks good but could be more thorough.The test verifies that nested YAML structures can be matched using the
$$$multi-metavariable. Consider extending this test to also verify:
- The contents of the nested sequence
- Matching specific items within the sequence
- Multiple levels of nesting
// Add after line 36 const sequenceNode = node4.find('$SEQ') assert.equal(sequenceNode.kind(), 'block_sequence') // Verify matching a specific item in the sequence const itemNode = root3.find('- item1') assert.equal(itemNode.kind(), 'block_sequence_item') assert.equal(itemNode.text().trim(), '- item1')
38-49: Replacement functionality test is implemented correctly.The test verifies that node replacement works properly by checking if the replacement string includes the expected transformed content. Consider strengthening this assertion to check the exact replaced text rather than just using
includes().- assert.equal(replaced.includes('value: key'), true) + // More precise assertion of the replaced content + const expectedReplacement = source.replace('key: value', 'value: key') + assert.equal(replaced, expectedReplacement)
🛑 Comments failed to post (1)
packages/yaml/nursery.js (1)
1-51: 🛠️ Refactor suggestion
Add error handling tests for malformed YAML.
To ensure the parser handles invalid inputs gracefully, consider adding tests for malformed YAML. This would verify that the error handling in the parser works as expected.
// Add before the closing brace on line 50 // Test error handling for malformed YAML try { const malformedYaml = ` key: - not valid indentation: wrong ` parse(malformedYaml) assert.fail('Should throw an error for malformed YAML') } catch (error) { // Just verify that an error was thrown assert.ok(error) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const { setup } = require('@ast-grep/nursery') const languageRegistration = require('./index') const assert = require('node:assert') setup({ dirname: __dirname, name: 'yaml', treeSitterPackage: '@tree-sitter-grammars/tree-sitter-yaml', languageRegistration, testRunner: (parse) => { // Test simple value matching const sg1 = parse('123') const root1 = sg1.root() const node1 = root1.find('123') assert.equal(node1.kind(), 'integer_scalar') // Test pattern matching with key-value pairs const sg2 = parse('foo: 123') const root2 = sg2.root() const node2 = root2.find('foo: $BAR') assert.equal(node2.kind(), 'block_mapping_pair') assert.equal(node2.find('$BAR').text(), '123') // Test non-matching pattern const node3 = root2.find('bar: $BAR') assert.equal(node3, null) // Test nested structures const sg3 = parse(` foo: - item1 - item2 `) const root3 = sg3.root() const node4 = root3.find('foo: $$$') assert.equal(node4.kind(), 'block_mapping_pair') // Test replacement functionality const source = ` key: value list: - item1 - item2 ` const sg4 = parse(source) const root4 = sg4.root() const node5 = root4.find('$KEY: value') const replaced = node5.replace('value: $KEY') assert.equal(replaced.includes('value: key'), true) // Test error handling for malformed YAML try { const malformedYaml = ` key: - not valid indentation: wrong ` parse(malformedYaml) assert.fail('Should throw an error for malformed YAML') } catch (error) { // Just verify that an error was thrown assert.ok(error) } } })
|
I cleaned it up, and took a shot at writing tests. |
|
Thanks for the contribution! The YAML test failed would you like to fix that? Otherwise the PR looks neat. |
|
Everything should be fixed! |
|
Thanks! |
|
package released |
I get this notice at the end of the create script, I'm not sure if it's important...
Related: ast-grep/ast-grep#1436
Summary by CodeRabbit
New Features
@ast-grep/lang-yamlpackage, providing YAML language support for AST-based code analysis tools.Documentation
Chores