-
Notifications
You must be signed in to change notification settings - Fork 6
✨ CSS #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ CSS #47
Conversation
WalkthroughThis pull request introduces a new CSS language package ( Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CSS as CSS Package
participant NAPI as @ast-grep/napi
participant Parser as Parser Engine
participant Tester as Nursery Test Runner
Dev->>CSS: Import CSS language module and functions
CSS->>NAPI: Register dynamic CSS language with registerDynamicLanguage()
Dev->>NAPI: Call parse() with CSS code snippet
NAPI->>Parser: Forward parsing request (using parser.so)
Parser-->>NAPI: Return syntax tree
NAPI-->>Dev: Deliver parsed AST
Tester->>Parser: Parse sample CSS rule from nursery.js
Tester-->>Tester: Assert AST root node type (e.g., 'rule_set')
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/css/nursery.js (1)
1-16
: Good test setup for basic CSS parsingThe test successfully validates that the parser can identify a CSS rule set. The setup correctly uses the nursery test framework with the appropriate configuration.
Consider expanding test coverage in the future to include more CSS constructs like:
- Complex selectors (pseudo-classes, combinators)
- Media queries
- Keyframe animations
- CSS variables
- Nested rules (for SCSS support)
This would ensure broader syntax coverage and more robust validation.
packages/css/README.md (2)
1-2
: Clarify the Package Title
The header in lines 1–2 is clear but could benefit from a slight rewording for clarity. Consider specifying that this package provides CSS language support using the NAPI approach (e.g., “ast-grep CSS language package using NAPI”).
14-24
: Usage Snippet Effectiveness
The usage block (lines 14–24) effectively demonstrates how to import, register, and use the new CSS language module. To further enhance clarity, you might add a brief inline comment explaining what the methodsg.root().kind()
returns. This would help users quickly grasp the purpose of that call.packages/css/package.json (3)
1-5
: Provide a Package Description
The basic metadata (lines 1–5) is set up correctly; however, the"description"
field is currently empty. Consider adding a brief description that clearly explains the purpose of the package (e.g., “Provides CSS language support for ast-grep with NAPI integration”).
6-12
: Review Script Commands for Consistency
The scripts block (lines 6–12) appears comprehensive. Note that both"source"
and"prepublishOnly"
runnode nursery.js source
, which might be intentional. It may help to add a comment or documentation in the repository explaining the rationale behind using the same command for both to avoid any confusion in future maintenance.
21-23
: Metadata Fields and Licensing
The"keywords"
,"author"
, and"license"
fields (lines 21–23) are present. While the"author"
field is empty, this can be filled as the project matures. Filling in the"description"
and"author"
fields would improve package metadata for end users and maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
packages/css/README.md
(1 hunks)packages/css/index.d.ts
(1 hunks)packages/css/index.js
(1 hunks)packages/css/nursery.js
(1 hunks)packages/css/package.json
(1 hunks)packages/css/postinstall.js
(1 hunks)
🔇 Additional comments (10)
packages/css/postinstall.js (1)
1-4
: Clean implementation following project patternsThe postinstall script correctly imports and uses the
postinstall
function from@ast-grep/setup-lang
with the current directory. This follows the established pattern for language packages in ast-grep.packages/css/index.d.ts (1)
1-10
: Well-structured type definitionThe
LanguageRegistration
type is properly defined with all the necessary properties for a language registration interface. The type properly distinguishes between required properties (libraryPath
,extensions
) and optional ones (marked with?
), and exports the registration constant as expected.packages/css/index.js (1)
1-9
:❓ Verification inconclusive
Correct language registration implementation
The implementation properly exports all required properties for CSS language registration, including the library path, supported file extensions, language symbol, and expando character.
Since 'scss' is included in the extensions array, please verify that the Tree-sitter CSS parser fully supports SCSS syntax. If the parser doesn't support SCSS-specific constructs (like nesting or variables), it might be better to limit to just 'css' or document the limitations.
🏁 Script executed:
#!/bin/bash # Check if tree-sitter-css documentation mentions SCSS support rg -i "scss" --glob="*.md" # Or check if there are SCSS-specific tests rg -i "scss" --glob="*.js" --glob="*.ts"Length of output: 120
Language Registration Verified; SCSS Support Requires Clarification
The implementation in
packages/css/index.js
correctly exports the required properties for CSS language registration, including the library path, supported file extensions, language symbol, and expando character.However, while the configuration registers both
"css"
and"scss"
, our search didn't reveal any additional tests or documentation confirming that the Tree-sitter CSS parser fully supports SCSS-specific syntax (such as nesting or variables). To ensure that the parser handles SCSS correctly, please verify whether:
- The parser includes support for SCSS-specific constructs beyond standard CSS.
- There are any tests or documentation updates that address potential limitations with SCSS.
If full SCSS support is not confirmed, consider either removing
"scss"
from the extensions array or documenting any limitations regarding SCSS.packages/css/README.md (1)
3-12
: Installation Instructions Are Clear
The installation section (lines 3–12) is well-documented and instructive. The inclusion of an inline comment for the optional installation oftree-sitter-cli
is helpful. Optionally, consider mentioning any version compatibility notes fortree-sitter-cli
if available.packages/css/package.json (6)
13-20
: Files Array Correctness
The"files"
array (lines 13–20) properly includes all required assets for the package. This ensures that consumers of the package will get the intended files, which is great.
24-26
: Dependency Declaration Review
The dependency on"@ast-grep/setup-lang": "0.0.3"
(lines 24–26) is clearly defined. Please verify that this version aligns with the rest of your toolchain and intended functionality.
27-34
: Peer Dependencies Validation
The configuration for peer dependencies (lines 27–34) correctly marks"tree-sitter-cli": "0.24.6"
as an optional peer dependency using"peerDependenciesMeta"
. This is in line with best practices for handling optional external tools.
35-39
: Dev Dependencies Are Appropriately Set
The dev dependencies (lines 35–39) include the necessary packages such as"@ast-grep/nursery"
,"tree-sitter-cli"
, and"tree-sitter-css"
, ensuring that development and testing are adequately supported.
40-43
: Publish Configuration is Correct
The"publishConfig"
block (lines 40–43) is set for public access with the correct registry URL. This configuration will help in the consistent publication of the package.
44-46
: PNPM Configuration Confirmation
The PNPM configuration (lines 44–46) with"onlyBuiltDependencies"
is correctly set to include only the built dependencies. This is a good practice to ensure that the final package is lean.
Here's CSS.
I'll add the changeset once the infrastructure for them is in place. It shouldn't be too hard to automate it for all the PRs with a little Bash.
Summary by CodeRabbit
New Features
Documentation