✨ HTML #51
✨ HTML #51
Conversation
WalkthroughThis pull request introduces a new package, Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI Install"
participant PI as "postinstall.js"
participant Setup as "@ast-grep/setup-lang"
participant HG as "@ast-grep/lang-html"
CLI->>PI: Execute installation
PI->>Setup: Call postinstall({ dirname })
Setup->>HG: Register HTML language
sequenceDiagram
participant Tester
participant Nursery as "nursery.js"
participant Parser as "HTML Parser"
Tester->>Nursery: Run HTML parsing test
Nursery->>Parser: Parse sample HTML string
Parser-->>Nursery: Return syntax tree
Nursery->>Tester: Confirm 'element' node found
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
🪧 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: 0
🧹 Nitpick comments (6)
.changeset/quick-pillows-find.md (1)
5-5: Enhance the changeset descriptionThe current description "Add @ast-grep/lang-html" is minimal. Consider providing more context about what this HTML language support enables for ast-grep users.
-Add @ast-grep/lang-html +Add @ast-grep/lang-html package for HTML language parsing and pattern matching supportpackages/html/README.md (3)
7-12: Consider adding instructions for npm and yarn usersThe installation instructions currently only mention pnpm. Consider adding equivalent commands for npm and yarn users to make the package more accessible.
```bash +# Using pnpm pnpm install @ast-grep/lang-html pnpm install @ast-grep/napi # install the tree-sitter-cli if no prebuild is available pnpm install @tree-sitter/cli --save-dev + +# Using npm +npm install @ast-grep/lang-html +npm install @ast-grep/napi +# install the tree-sitter-cli if no prebuild is available +npm install @tree-sitter/cli --save-dev + +# Using yarn +yarn add @ast-grep/lang-html +yarn add @ast-grep/napi +# install the tree-sitter-cli if no prebuild is available +yarn add @tree-sitter/cli --dev--- `16-24`: **Enhance usage example with a realistic HTML snippet** The current example uses a placeholder string (`"your code"`) for parsing. Consider using a simple but realistic HTML snippet to demonstrate the parser's capabilities and what users can expect from the output. ```diff ```js import html from '@ast-grep/lang-html' import { registerDynamicLanguage, parse } from '@ast-grep/napi' registerDynamicLanguage({ html }) -const sg = parse('html', `your code`) -sg.root().kind() +// Parse a simple HTML snippet +const sg = parse('html', `<div class="container"><p>Hello World</p></div>`) + +// Get the root node kind +const rootKind = sg.root().kind() +console.log('Root node kind:', rootKind) + +// Example of traversing the HTML structure +const paragraphs = sg.root().findAll('tag_name: p') +console.log('Found paragraphs:', paragraphs.length)--- `1-25`: **Add explanation about tree-sitter integration** The README doesn't explain the relationship with tree-sitter or why the CLI might be needed. Consider adding a brief explanation about how ast-grep uses tree-sitter for parsing HTML. Add a section after the Installation section: ```markdown ## About This package provides HTML language support for ast-grep using tree-sitter. The tree-sitter parser allows ast-grep to understand and query HTML syntax trees, enabling powerful pattern matching and code transformation capabilities for HTML files. If no prebuilt binaries are available for your platform, the `@tree-sitter/cli` package is required to build the parser locally during installation.packages/html/package.json (2)
4-4: Add a descriptive package description.The description field is currently empty. Adding a clear description will help users understand the purpose of this package when viewing it on npm or GitHub.
- "description": "", + "description": "HTML language support for ast-grep",
22-23:❓ Verification inconclusive
Consider adding author information and using a more specific license.
The author field is empty, and ISC license is used. Consider:
- Adding author information (e.g., "ast-grep team" or individual contributors)
- Using the same license as other ast-grep packages for consistency
- "author": "", - "license": "ISC", + "author": "ast-grep team", + "license": "MIT",
🏁 Script executed:
#!/bin/bash # Check what license other ast-grep packages use fd "package.json" --type f --path packages | xargs grep -l "\"license\":" | xargs grep "\"license\":"Length of output: 331
Below is a new shell script to verify the license and author fields in all
package.jsonfiles under thepackagesdirectory. The previous command failed due to an invalid flag; this version omits the problematic argument:Action:
The review comment recommends updatingpackages/html/package.jsonby adding an author (e.g.,"ast-grep team") and switching from theISClicense toMIT(to match other ast-grep packages). Please run the above script manually to verify the values used in the other packages. Once you confirm whether those files indeed use"author": "ast-grep team"and"license": "MIT", we can finalize the update.
🏁 Script executed:
#!/bin/bash # Check license and author entries in all package.json files under the packages directory fd "package.json" --type f packages | while read file; do echo "==> $file" grep '"author":' "$file" grep '"license":' "$file" doneLength of output: 2077
Action Required: Confirm Consistent Package Metadata Updates
Inpackages/html/package.json, theauthorfield is empty and thelicenseis set to"ISC", which is consistent with all other packages (see output from Angular, C, C++, C#, Go, Python, SQL, and TOML packages). The original suggestion to change these values to"ast-grep team"and"MIT"would introduce inconsistency unless the entire repository is updated.
- Recommendation:
Either update all
package.jsonfiles across the repository to use:
"author": "ast-grep team""license": "MIT"Diff snippet (if updating all):
- "author": "", - "license": "ISC", + "author": "ast-grep team", + "license": "MIT",Or retain the current settings for consistency, and consider addressing author information in a separate, comprehensive update.
Please verify the intended repository-wide metadata strategy and update accordingly.
📜 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 (7)
.changeset/quick-pillows-find.md(1 hunks)packages/html/README.md(1 hunks)packages/html/index.d.ts(1 hunks)packages/html/index.js(1 hunks)packages/html/nursery.js(1 hunks)packages/html/package.json(1 hunks)packages/html/postinstall.js(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/html/postinstall.js (1)
packages/html/nursery.js (1)
require(1-1)
🔇 Additional comments (8)
packages/html/postinstall.js (1)
1-4: LGTM! Post-installation script follows package conventionsThe post-installation script correctly imports and calls the
postinstallfunction from the@ast-grep/setup-langpackage, passing the current directory.packages/html/index.d.ts (2)
1-7: LGTM! Type definition is well-structuredThe
LanguageRegistrationtype is well-defined with appropriate property types and optional flags.
9-10: LGTM! Export follows TypeScript best practicesThe declaration and export of the
registrationconstant follows TypeScript best practices.packages/html/index.js (1)
1-9: LGTM! Well-structured module for HTML language registration.The implementation correctly exports the required properties for HTML language registration in ast-grep:
- Uses the proper path resolution for the parser library
- Includes appropriate file extensions (html, htm, xhtml)
- Sets the correct language symbol and expando character
This aligns with the standard pattern for language packages in the ast-grep ecosystem.
packages/html/nursery.js (1)
1-16: LGTM! Good test setup for HTML parsing verification.The test case appropriately validates that:
- The HTML parser can correctly parse a simple HTML element
- The pattern matching functionality works with HTML syntax
- The node identification is accurate (checking that 'element' kind is recognized)
This is a solid minimal test to ensure the HTML language support works correctly.
packages/html/package.json (3)
1-47: LGTM overall! Package configuration follows the project's patterns.The package.json is well-structured with:
- Appropriate scripts for build, test, and publishing
- Correct dependencies and peer dependencies
- Proper file inclusion list
- Publishing configuration
Minor suggestions noted in separate comments.
10-10:❓ Verification inconclusive
Verify that postinstall.js exists.
The
postinstallscript referencespostinstall.js, which is included in thefilesarray but not provided in the PR.
🏁 Script executed:
#!/bin/bash # Check if postinstall.js exists in the PR fd --type f --exact-path "packages/html/postinstall.js" # If it doesn't exist, check if it's in the repo but not in the PR if [ $? -ne 0 ]; then echo "postinstall.js not found in the PR, checking if it exists in other packages:" fd "postinstall.js" --type f --path packages fiLength of output: 745
Verify that
postinstall.jsexists in thepackages/htmlfolder.
- The
postinstallscript inpackages/html/package.jsonreferencespostinstall.js, but our initial automated check didn’t produce conclusive results.- The original
fdcommand failed due to incorrect flags (--exact-pathand--path), so the file wasn’t reliably located.- Action Required: Please verify manually (or with a corrected script, e.g., using
find packages/html -type f -name "postinstall.js") thatpostinstall.jsexists in the intended location or is intentionally omitted.
44-46:❓ Verification inconclusive
Verify the pnpm.onlyBuiltDependencies config is correct.
The config includes both
@ast-grep/lang-html(the package itself) andtree-sitter-cli. Make sure that including the package itself inonlyBuiltDependenciesfollows the pattern used in other language packages.
🏁 Script executed:
#!/bin/bash # Check what other language packages include in onlyBuiltDependencies echo "Checking onlyBuiltDependencies in other language packages:" fd "package.json" --type f --path packages | xargs grep -l "onlyBuiltDependencies" | xargs grep -A3 "onlyBuiltDependencies"Length of output: 480
Action: Verify the pnpm.onlyBuiltDependencies configuration
The configuration in
packages/html/package.jsoncurrently lists both@ast-grep/lang-htmlandtree-sitter-cli. An initial attempt to check how similar language packages use this configuration ran into an error (the--pathflag isn’t valid for thefdcommand), so we couldn’t automatically confirm whether including the package itself follows the established pattern.Please manually verify that:
- Including the package itself (i.e.
@ast-grep/lang-html) inonlyBuiltDependenciesis intentional.- The configuration is consistent with how dependencies are handled in similar language packages.
If this pattern is unique or incorrect compared to other language packages, adjust the configuration accordingly.
Summary by CodeRabbit
@ast-grep/lang-htmlfor HTML language support and parsing.LanguageRegistrationtype and aregistrationconstant for dynamic language registration.@ast-grep/lang-htmlpackage.