-
Notifications
You must be signed in to change notification settings - Fork 62
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
fix: harden and standardize edit_grammars script #412
Conversation
WalkthroughWalkthroughThe changes introduced in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as edit_grammars.mjs
participant Filesystem as Filesystem
User->>Script: Run script
Script->>Script: Initialize constants and language lists
Script->>Filesystem: Update file paths using fileURLToPath
Script->>Script: Modify execPromise to accept cwd
Script->>Filesystem: Copy build files
Script->>Script: Restructure build steps for languages
Script->>Filesystem: Update language directories and files
Script->>Filesystem: Add new grammar tree-sitter-toml
Script-->>User: Script execution completed
sequenceDiagram
participant Developer
participant Cargo as Cargo.toml
participant Makefile as Makefile
participant Node as binding.gyp
participant Python as setup.py
Developer->>Cargo: Update description, keywords, edition
Developer->>Makefile: Define build configurations
Developer->>Node: Update dependencies, source files
Developer->>Python: Add build and package logic
Python-->>Developer: Initialize Python extension module
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (4)
Files selected for processing (31)
Files not summarized due to errors (1)
Files skipped from review due to trivial changes (14)
Additional context usedLearnings (1)
LanguageTool
Additional comments not posted (38)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration 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.
25 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
|
||
// Get the tree-sitter Language for this grammar. | ||
func Language() unsafe.Pointer { | ||
return unsafe.Pointer(C.tree_sitter_toml()) |
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.
🪶 style: Consider handling potential errors from C.tree_sitter_toml()
to avoid unsafe pointer dereferencing.
{"language", _binding_language, METH_NOARGS, | ||
"Get the tree-sitter language for this grammar."}, |
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.
🪶 style: Consider adding error handling for tree_sitter_toml()
return value.
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: 3
Outside diff range, codebase verification and nitpick comments (1)
resources/language-metavariables/tree-sitter-toml/corpus/spec.txt (1)
424-424
: Avoid repetition in phrasing.The phrase "This," she said, "is just a pointless statement." is repeated. Consider rephrasing to add variety.
- str7 = """"This," she said, "is just a pointless statement."""" + str7 = """"This," she said, "is merely a pointless statement.""""Tools
LanguageTool
[style] ~424-~424: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ement." str7 = """"This," she said, "is just a pointless statement."""" -----------...(REP_BE_JUST)
# [x.y.z] need these | ||
[x.y.z.w] # for this to work | ||
|
||
[x] # defining a super-table afterwards is ok |
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.
Use "afterward" for American English.
In American English, ‘afterward’ is the preferred variant.
- [x] # defining a super-table afterwards is ok
+ [x] # defining a super-table afterward is ok
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.
[x] # defining a super-table afterwards is ok | |
[x] # defining a super-table afterward is ok |
Tools
LanguageTool
[locale-violation] ~1042-~1042: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...s to work [x] # defining a super-table afterwards is ok --------------------------------...(AFTERWARDS_US)
================================================================================ | ||
|
||
# What you see is what you get. | ||
winpath = 'C:\Users\nodejs\templates' |
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.
Correct spelling for "Node.js".
The official spelling of this programming framework is “Node.js”.
- winpath = 'C:\Users\nodejs\templates'
+ winpath = 'C:\Users\Node.js\templates'
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.
winpath = 'C:\Users\nodejs\templates' | |
winpath = 'C:\Users\Node.js\templates' |
Tools
LanguageTool
[uncategorized] ~455-~455: The official spelling of this programming framework is “Node.js”.
Context: ...e is what you get. winpath = 'C:\Users\nodejs\templates' winpath2 = '\ServerX\admin$...(NODE_JS)
VALID - string - multi-line literal strings | ||
================================================================================ | ||
|
||
regex2 = '''I [dw]on't need \d{2} apples''' |
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.
Correct potential typo.
The phrase "I [dw]on't need" might be a typo. Consider correcting it to "don't" or "won't".
- regex2 = '''I [dw]on't need \d{2} apples'''
+ regex2 = '''I don't need \d{2} apples'''
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.
regex2 = '''I [dw]on't need \d{2} apples''' | |
regex2 = '''I don't need \d{2} apples''' |
Tools
LanguageTool
[uncategorized] ~481-~481: Did you mean “don't” or “won't”?
Context: ...==================== regex2 = '''I [dw]on't need \d{2} apples''' lines = ''' The f...(ONT_DONT)
Our yaml grammar is currently unbuildable, which we didn't catch earlier due to oddness with the edit_grammars script.
This PR attempts to update the edit_grammars script to rely less on relative paths.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Greptile Summary
This is an auto-generated summary
resources/edit_grammars.mjs
using Node.js path utilitiesCargo.toml
for tree-sitter-toml with precise dependencies and edition 2021Makefile
andPackage.swift
for tree-sitter-toml to standardize build processesbindings/node/binding.cc
to usenapi
library