Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the parsing code to support custom extensions while preserving existing functionality. The refactoring enables the Parsons Problems group to add custom parsing logic for template solutions without modifying core repository code.
Key changes:
- Refactors the monolithic parser into modular token and parse handlers with extensible interfaces
- Introduces handler-based architecture allowing custom tokenizer and parser extensions to be injected
- Adds comprehensive test coverage for the tokenizer functionality
Reviewed Changes
Copilot reviewed 11 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/parser/parser.ts | Main parser logic extracted from original parser.ts with extensibility support |
| src/parser/tokenhandler.ts | Token handling logic with pluggable handler interface |
| src/parser/parsehandler.ts | Parse handling logic with pluggable handler interface |
| test/parser.test.ts | New test suite covering tokenizer functionality |
| src/web/ide.ts | Updated import path and commented out AST description functionality |
| src/scamper.ts | Updated import path to new parser location |
| src/ast.ts | Enhanced AST node handling and commented out description method |
| package.json | Added Jest globals dependency and updated test script |
| jest.config.ts | Added ESM support configuration |
| pnpm-workspace.yaml | Added workspace configuration |
| src/parser.ts | Removed original monolithic parser file |
| /*const descriptionEl = document.createElement('div') | ||
| descriptionEl.setAttribute('id', 'ast-desc') | ||
| descriptionEl.innerText = parsed.ast.describe() | ||
| descriptionEl.setAttribute('tabindex', '0') | ||
| descriptionEl.setAttribute('role', 'region') | ||
| outputPane!.appendChild(descriptionEl) | ||
| outputPane!.appendChild(descriptionEl)*/ |
There was a problem hiding this comment.
Instead of commenting out code, consider removing it entirely or using a feature flag if this functionality might be restored later.
| //name: string = '' | ||
| //parentname: string | null = null; |
There was a problem hiding this comment.
Remove commented-out code rather than leaving it in the codebase. If this property might be needed later, consider using a feature flag or documenting the decision.
| //name: string = '' | |
| //parentname: string | null = null; |
| // the entire sequence and let Javascript handle interpreting the | ||
| // sequence for us! | ||
| } else if (ch === '\\') { | ||
| tokenizer.advance() // advance past '\\ |
There was a problem hiding this comment.
The comment has an incorrect escape sequence. It should be "advance past '\'" with proper escaping.
| tokenizer.advance() // advance past '\\ | |
| tokenizer.advance() // advance past '\\' |
| // NOTE: skip the extra \ that we parsed in this case. If/when we support | ||
| // extended escape codes, the size of the jump will obviously grow! |
There was a problem hiding this comment.
[nitpick] The comment refers to skipping "the extra \" but should clarify it's skipping the escaped character, not just the backslash.
| // NOTE: skip the extra \ that we parsed in this case. If/when we support | |
| // extended escape codes, the size of the jump will obviously grow! | |
| // NOTE: skip the escaped character (including the backslash) that we parsed in this case. | |
| // If/when we support extended escape codes, the size of the jump will obviously grow! |
| @@ -0,0 +1,38 @@ | |||
| import {stringToTokens} from "../src/parser/parser"; | |||
There was a problem hiding this comment.
[nitpick] Consider importing only the specific functions needed rather than destructuring from a large module, or use a barrel export for better organization.
The Parsons Problems group in Glimmer is looking to support Parsons Problem generation based on template solutions.
For example, say we have the template Scamper solution:
We hope for this to generate the following blocks:
In order to accomplish our goals, we need a custom Scamper parser to support our special syntax:
This PR:
Thus, no functionality/Parsons Problem-specific code gets added to this repository, but it allows us to reuse the existing tokenizer/parser.