Refactoring formatter - #15
Conversation
Summary of ChangesHello @abco20, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the BT DSL formatter by migrating its underlying parsing technology. It transitions from a Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring that replaces the Tree-sitter based parser in the formatter with a new one based on the core C++ parser compiled to WASM. This is a great move for consolidating the language grammar and likely improving performance. The changes are extensive, touching everything from the parser logic and AST structure to build scripts and documentation. The new implementation in prettier-plugin-bt-dsl.ts is impressive, with robust error handling, UTF-8 to UTF-16 offset conversion, and careful comment preservation. The addition of new tests for malformed input and other edge cases is also excellent.
I've added a few suggestions to improve the maintainability of the build and test scripts, and to clean up some leftover debug code in one of the new tests. Overall, this is a very well-executed and substantial improvement to the formatter.
| "lint:fix": "pnpm -w exec eslint formatter/src formatter/test --max-warnings 0 --fix", | ||
| "pretest": "pnpm build", | ||
| "test": "node dist/test/test-format.js && node dist/test/test-standard-nodes.js && node dist/test/test-params.js && node dist/test/test-comment-preservation.js && node dist/test/test-extern-parens.js" | ||
| "test": "node dist/test/test-format.js && node dist/test/test-standard-nodes.js && node dist/test/test-params.js && node dist/test/test-comment-preservation.js && node dist/test/test-extern-parens.js && node dist/test/test-wasm-relocation.js && node dist/test/test-malformed-input.js && node dist/test/test-trailing-comments.js && node dist/test/test-argument-comments.js" |
There was a problem hiding this comment.
The test script is becoming long as it manually lists every test file. This can be brittle and harder to maintain as more tests are added.
Consider using a test runner that can automatically discover and run tests. Since other parts of the project use node --test, you could adopt it here as well. This would likely involve renaming your test files to follow a pattern like *.test.js or *.test.mjs so you can use a glob pattern.
For example, if you rename the test files, you could simplify the script to something like:
"test": "node --test dist/test/"This would make the test execution more robust and easier to manage.
| console.log('DEBUG: input =', JSON.stringify(input)); | ||
| console.log('DEBUG: output =', JSON.stringify(out)); | ||
| console.log('DEBUG: 日本語コメント count =', countOccurrences(out, '日本語コメント')); | ||
| console.log('DEBUG: AlwaysSuccess(); count =', countOccurrences(out, 'AlwaysSuccess();')); |
There was a problem hiding this comment.
These console.log statements appear to be for debugging. It's best to remove them to keep the test output clean.
| "scripts": { | ||
| "prebuild": "(if [ -f ../core/build-lsp/bt_dsl_lsp_server ]; then mkdir -p server && cp -f ../core/build-lsp/bt_dsl_lsp_server server/; elif [ -f ../core/build/bt_dsl_lsp_server ]; then mkdir -p server && cp -f ../core/build/bt_dsl_lsp_server server/; elif [ -f server/bt_dsl_lsp_server ]; then :; else echo 'Warning: LSP server not found (checked ../core/build-lsp/bt_dsl_lsp_server, ../core/build/bt_dsl_lsp_server, server/bt_dsl_lsp_server). Set BT_DSL_LSP_PATH for dev or build bt_dsl_lsp_server.'; fi) && if [ -d ../core/std ]; then cp -rf ../core/std .; else echo 'Warning: std/ not found'; fi && if [ -f ../tree-sitter-bt-dsl/tree-sitter-bt_dsl.wasm ]; then mkdir -p out && cp ../tree-sitter-bt-dsl/tree-sitter-bt_dsl.wasm out/; else echo 'Warning: tree-sitter WASM not found for formatter.'; fi", | ||
| "build": "tsc -b && esbuild src/extension.ts --bundle --outfile=out/extension.js --external:vscode --external:vscode-languageclient --external:vscode-languageclient/* --external:@bt-dsl/formatter --external:web-tree-sitter --external:prettier --format=esm --platform=node", | ||
| "prebuild": "(if [ -f ../core/build-lsp/bt_dsl_lsp_server ]; then mkdir -p server && cp -f ../core/build-lsp/bt_dsl_lsp_server server/; elif [ -f ../core/build/bt_dsl_lsp_server ]; then mkdir -p server && cp -f ../core/build/bt_dsl_lsp_server server/; elif [ -f server/bt_dsl_lsp_server ]; then :; else echo 'Warning: LSP server not found (checked ../core/build-lsp/bt_dsl_lsp_server, ../core/build/bt_dsl_lsp_server, server/bt_dsl_lsp_server). Set BT_DSL_LSP_PATH for dev or build bt_dsl_lsp_server.'; fi) && if [ -d ../core/std ]; then cp -rf ../core/std .; else echo 'Warning: std/ not found'; fi && pnpm --filter @bt-dsl/core build:wasm && mkdir -p out && cp -f ../core/build-wasm/formatter_wasm.js out/ && cp -f ../core/build-wasm/formatter_wasm.wasm out/", |
There was a problem hiding this comment.
The prebuild script is quite long and contains complex shell logic, which can be difficult to read and maintain within a JSON file.
For better readability and maintainability, consider moving this logic into a separate shell script file, for example, scripts/prebuild.sh.
You could create a scripts/prebuild.sh file with the logic and then call it from package.json:
"prebuild": "./scripts/prebuild.sh"This would make the package.json cleaner and the script easier to manage.
1 participant
No description provided.