Add parsing for end of comment when using whitespace control + add tooling for testing#113
Merged
Merged
Conversation
…oling for testing
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates Handlebars grammar patterns for tilde-aware comment delimiters and anchored YAML front-matter delimiters across all supported grammar formats. Adds a Node-based tokenization test harness, CI workflow, and test coverage for comments, expressions, blocks, and embedding. ChangesHandlebars Grammar Fix and Test Suite
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 16-23: The test workflow currently uses actions/checkout@v4 with
default credential persistence, which leaves write-capable git credentials
available to PR-controlled steps. Update the checkout step to disable persisted
credentials, and adjust the job permissions so it only has read-only contents
access. Keep the change localized to the workflow job that runs npm ci and npm
test.
In `@test/blocks.test.js`:
- Around line 68-74: The whitespace-control block assertions in blocks.test.js
are using the wrong token text for the opening delimiter. Update the relevant
assertScope calls in the whitespace-control block test to look up the exact
`{{~` token text, matching the behavior already used in the expression
whitespace-control suite, so `scopesOf()` can find the token consistently for
both the block start and end cases.
In `@test/embedding.test.js`:
- Around line 44-51: The scope assertion in tokenizeLines() is too broad because
lines.flat() includes tokens from the <script> opener and </script> closer, not
just the embedded template body. Update the test around tokenizeLines(src) to
only iterate over the script body tokens, using the existing src fixture and the
relevant token/scoping checks in test/embedding.test.js, so the
source.handlebars.embedded.html assertion applies only to the embedded content.
In `@test/helpers/grammar.js`:
- Around line 26-33: Update the grammar loader in `test/helpers/grammar.js` so
`loadGrammar` can serve every external scope referenced by
`grammars/Handlebars.json`, not just `SCOPE_NAME`. Extend the `vsctm.Registry`
setup to map and return parsed grammars for `text.html.handlebars`,
`text.html.basic`, `source.css`, and `source.js`, using the existing
`vsctm.parseRawGrammar`/`fs.readFileSync` flow so the tokenizer harness resolves
embedded HTML, CSS, and JS paths correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d52aa1e-87b1-4875-ae04-a7bda0dbedd6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.github/workflows/test.yml.gitignoregrammars/Handlebars.jsongrammars/Handlebars.sublime-syntaxgrammars/Handlebars.tmLanguagepackage.jsontest/blocks.test.jstest/comments.test.jstest/embedding.test.jstest/expressions.test.jstest/helpers/grammar.js
Comment on lines
+68
to
+74
| test('whitespace control on a block: {{~#if x~}} ... {{~/if~}}', async () => { | ||
| await assertScope('{{~#if x~}}', '{{', 'meta.function.block.start.handlebars'); | ||
| await assertScope('{{~#if x~}}', '~#', 'keyword.control'); | ||
| await assertScope('{{~#if x~}}', '~}}', 'support.constant.handlebars'); | ||
| await assertScope('{{~/if~}}', '{{', 'meta.function.block.end.handlebars'); | ||
| await assertScope('{{~/if~}}', '~/', 'keyword.control'); | ||
| await assertScope('{{~/if~}}', '~}}', 'support.constant.handlebars'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the exact {{~ token text in the whitespace-control block assertions.
scopesOf() does an exact t.text lookup, and the sibling expression suite already treats the whitespace-control prefix as {{~ (test/expressions.test.js:70). With the current {{ lookup here, these assertions will throw no token with text "{{" as soon as block delimiters are tokenized the same way.
Suggested fix
test('whitespace control on a block: {{~`#if` x~}} ... {{~/if~}}', async () => {
- await assertScope('{{~`#if` x~}}', '{{', 'meta.function.block.start.handlebars');
+ await assertScope('{{~`#if` x~}}', '{{~', 'meta.function.block.start.handlebars');
await assertScope('{{~`#if` x~}}', '~#', 'keyword.control');
await assertScope('{{~`#if` x~}}', '~}}', 'support.constant.handlebars');
- await assertScope('{{~/if~}}', '{{', 'meta.function.block.end.handlebars');
+ await assertScope('{{~/if~}}', '{{~', 'meta.function.block.end.handlebars');
await assertScope('{{~/if~}}', '~/', 'keyword.control');
await assertScope('{{~/if~}}', '~}}', 'support.constant.handlebars');
});📝 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.
Suggested change
| test('whitespace control on a block: {{~#if x~}} ... {{~/if~}}', async () => { | |
| await assertScope('{{~#if x~}}', '{{', 'meta.function.block.start.handlebars'); | |
| await assertScope('{{~#if x~}}', '~#', 'keyword.control'); | |
| await assertScope('{{~#if x~}}', '~}}', 'support.constant.handlebars'); | |
| await assertScope('{{~/if~}}', '{{', 'meta.function.block.end.handlebars'); | |
| await assertScope('{{~/if~}}', '~/', 'keyword.control'); | |
| await assertScope('{{~/if~}}', '~}}', 'support.constant.handlebars'); | |
| test('whitespace control on a block: {{~`#if` x~}} ... {{~/if~}}', async () => { | |
| await assertScope('{{~`#if` x~}}', '{{~', 'meta.function.block.start.handlebars'); | |
| await assertScope('{{~`#if` x~}}', '~#', 'keyword.control'); | |
| await assertScope('{{~`#if` x~}}', '~}}', 'support.constant.handlebars'); | |
| await assertScope('{{~/if~}}', '{{~', 'meta.function.block.end.handlebars'); | |
| await assertScope('{{~/if~}}', '~/', 'keyword.control'); | |
| await assertScope('{{~/if~}}', '~}}', 'support.constant.handlebars'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/blocks.test.js` around lines 68 - 74, The whitespace-control block
assertions in blocks.test.js are using the wrong token text for the opening
delimiter. Update the relevant assertScope calls in the whitespace-control block
test to look up the exact `{{~` token text, matching the behavior already used
in the expression whitespace-control suite, so `scopesOf()` can find the token
consistently for both the block start and end cases.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/helpers/grammar.js`:
- Around line 28-43: The catch-all fallback in loadGrammar is masking unexpected
scope references and can hide broken grammar changes. Update the
loadGrammar(scopeName) logic in test/helpers/grammar.js to return the empty stub
only for the known built-in scopes used by the grammar (text.html.basic,
source.css, source.js, source.yaml), and fail fast by throwing for any other
unknown scope so typos and newly introduced missing includes are caught by the
test harness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 974927ee-eecc-47cf-9a45-cc4c1b8fa158
📒 Files selected for processing (6)
.github/workflows/test.ymlgrammars/Handlebars.jsongrammars/Handlebars.sublime-syntaxgrammars/Handlebars.tmLanguagetest/embedding.test.jstest/helpers/grammar.js
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #112
Summary by CodeRabbit
~) in inline/block comments and block tags.---delimiter detection and refined embedded Handlebars highlighting in surrounding HTML/script contexts.testscript; updated.gitignoreto excludenode_modules/.