feat: add shell highlight hover, document symbol and link feature to snakemake #45
Conversation
📝 WalkthroughWalkthroughThis update introduces significant enhancements to the Snakemake Visual Studio Code extension. A new TypeScript-based extension implementation is added, featuring a Changes
Sequence Diagram(s)sequenceDiagram
participant VSCode
participant SnakemakeManager
participant FileSystem
VSCode->>SnakemakeManager: On document open/change
SnakemakeManager->>FileSystem: Read document content
SnakemakeManager->>SnakemakeManager: Parse for rules, modules, shell blocks, links
SnakemakeManager->>VSCode: Provide symbols and document links
VSCode->>SnakemakeManager: On hover over shell block
SnakemakeManager->>VSCode: Return shell block content as hover tooltip
sequenceDiagram
participant Developer
participant VSCode Tasks
participant build.mjs
participant esbuild
Developer->>VSCode Tasks: Run "build" task
VSCode Tasks->>build.mjs: Execute build script
build.mjs->>esbuild: Bundle and minify src/extension.ts
esbuild-->>build.mjs: Output dist/extension.js
build.mjs-->>VSCode Tasks: Build complete
Possibly related PRs
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
syntaxes/snakemake.tmLanguage.json (1)
257-278: 🛠️ Refactor suggestionSimplify optional
with:handling inuserulesfromas
Embedding a free‑spacing comment inside the JSON regex can lead to escaping pitfalls. Instead of the inline comment, capture the optionalwith:clause directly:- "match": "(?x)\n \\b(use\\s+rule)\\s+(\\w+|\\w+\\*|\\*)\\s+(from)\\s+(\\w+)\\s+(as)\\s+(\\w+|\\w+\\*)\n # may end with a \"with:\"\", but that can be caught by other rules\n", + "match": "(?x)\n \\b(use\\s+rule)\\s+(\\w+|\\w+\\*|\\*)\\s+(from)\\s+(\\w+)\\s+(as)\\s+(\\w+|\\w+\\*)\n (?:\\s+with:)?\n",This explicit optional group is easier to maintain and avoids JSON‑escaping complexity.
🧹 Nitpick comments (12)
src/js/build_regex.js (1)
10-10: Ensure safe YAML parsing with explicit schema
Switching from the deprecatedsafeLoadtoloadis correct, but by defaultyaml.loadmay allow unsafe types. To preserve the original safe behavior, explicitly pass a safe schema, for example:const config = yaml.load(fs.readFileSync(keywords, 'utf8'), { schema: yaml.JSON_SCHEMA });build.mjs (1)
1-12: Consider adding inline source maps for debugging
To improve debugging of the bundled extension, you could enable inline source maps:esbuild.buildSync({ entryPoints: ['src/extension.ts'], bundle: true, platform: 'node', outfile: 'dist/extension.js', external: ['vscode', 'node'], format: 'cjs', minify: true, + sourcemap: 'inline', });This will help trace runtime errors back to the original TypeScript sources.
.vscode/tasks.json (1)
25-29: Simplify js_build command
Usingnpx node build.mjsworks, but invoking Node directly is sufficient. You can simplify the task to:"command": "node build.mjs"avoiding the extra
npxlayer..github/workflows/release-please.yml (1)
26-33: Add anactions/setup-nodecache for faster releasesEvery release run currently installs all npm packages from scratch.
Caching$HOME/.npm(or~/.cache/npm) withcache: "npm"insetup-node@v4cuts the release time noticeably, especially with the newly‑added build step.Example:
- uses: actions/setup-node@v4 if: ${{ steps.release.outputs.release_created }} with: node-version: 18 cache: 'npm'README.md (2)
49-56: Minor wording / formatting glitchThe sentence starts with “You can re-
used rule …”, which looks like an unfinished edit.
Consider re‑phrasing to:- - You can re- `used rule <rulename> as <newrulename> ... with` + - You can reuse a rule via + ```snakemake + use rule <rulename> as <newrulename> ... + ```
104-105: Convert TODOs into tracked GitHub issuesThe README TODO list is easy to forget. Creating GitHub issues (labelled e.g. enhancement) provides visibility and lets the community pick them up.
src/extension.ts (2)
205-213: Avoid synchronous filesystem checks inside the hot parsing loop
fs.existsSyncis called for every candidate path on every document change, blocking the extension host thread.
Either:
- Cache already‑tested paths in a
WeakMap, or- Switch to
vscode.workspace.fs.stat(async) and throttle look‑ups.This will keep typing latency low for large Snakefiles.
59-67: Full document re‑parse on each keystroke can hurt performance
update()walks the entire file ononDidChangeTextDocument, even for single‑character edits. Consider debouncing and/or incremental parsing to avoid quadratic behaviour in large files.syntaxes/snakemake.tmLanguage.json (2)
53-62: Capture function name and paren for highlighting
Thefunction-callblock correctly matchesname(args)but lacks abeginCapturesto scope the function name (group 1) and the opening parenthesis. Adding these will improve syntax highlighting consistency:"function-call": { "begin": "(?x)\n \\b(?=\n ([[:alpha:]_]\\w*) \\s* (\\()\n )\n", + "beginCaptures": { + "1": { "name": "entity.name.function.snakemake.call" }, + "2": { "name": "punctuation.definition.arguments.begin.python" } + }, "end": "(\\))", "endCaptures": { ... }, "patterns": [ ... ] }
350-362: Typo inrulesrefernceidentifier
The key and references userulesrefernce(missing the secondein “reference”). While this is internally consistent, renaming torulesreferencewould improve readability and avoid confusion.src/yaml/snakemake.syntax.yaml (2)
23-40: Function-call lacks beginCaptures for function name
Thefunction-callentry correctly matches calls but does not capture the function name (group 1) or opening parenthesis. Adding:beginCaptures: "1": { name: entity.name.function.snakemake.call } "2": { name: punctuation.definition.arguments.begin.python }will enable proper token scoping.
198-204: Typo inrulesreferncekey
The pattern is namedrulesreferncebut should likely berulesreference. Renaming will enhance readability and align with standard spelling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.github/workflows/release-please.yml(1 hunks).vscode/launch.json(1 hunks).vscode/tasks.json(1 hunks)README.md(1 hunks)build.mjs(1 hunks)package.json(1 hunks)src/extension.ts(1 hunks)src/js/build_regex.js(1 hunks)src/yaml/snakemake.syntax.yaml(4 hunks)syntaxes/snakemake.tmLanguage.json(10 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/extension.ts
[error] 155-155: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (23)
.vscode/launch.json (1)
6-6: Styling-only change
This whitespace adjustment is purely cosmetic and doesn’t affect functionality.tsconfig.json (1)
1-21: TypeScript configuration is well-defined
Thetsconfig.jsonsettings align with the new TypeScript build process, enabling strict checks, proper module resolution, and excluding build artifacts..vscode/tasks.json (1)
31-37: Integrate js_build into the composite build task
The addition ofjs_buildto thebuildtask dependencies ensures the TypeScript bundle step is run alongside the existing build tasks, aligning VSCode’s task configuration with your updated CI/build pipeline..github/workflows/release-please.yml (1)
18-21: Branch still hard‑coded tomasterIf the repository has transitioned to
main, or plans to, this trigger will silently stop firing.
Double‑check thatmasteris indeed the default branch.src/extension.ts (1)
149-156: Remove superfluouscontinueThe
continueat line 155 is flagged by Biome and is indeed redundant—the loop proceeds to the next iteration naturally after the block.
Deleting it cleans up the control flow.- this.pushAnyBlocks(blocks, state, i, line, state.blockStart.character); - continue; + this.pushAnyBlocks(blocks, state, i, line, state.blockStart.character);🧰 Tools
🪛 Biome (1.9.4)
[error] 155-155: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
syntaxes/snakemake.tmLanguage.json (9)
10-11: New repository pattern includes correctly integrated
The#snakestrings,#function-call,#quoteall, and#snakemakenamespatterns have been added to the top‑levelpatternsarray in the correct order, ensuring that strings, function calls, nested quotes, and semantic name groups are tokenized before falling back tosource.python.Also applies to: 40-41, 43-44, 46-47
81-148: Function-arguments patterns are comprehensive
Thefunction-argumentsrepository entry cleanly handles argument separators, unpacking (*,**), named parameters, nested expressions, and reinserts#snakemakenamesand string patterns as needed. The lookahead anchors and content naming align with best practices.
149-161: Consolidated quoting patterns correctly defined
Thequoteallpattern aggregatesquotessmall,quotesmid, andquotesbigto support nested structures, simplifying the overall grammar.
162-175: Parenthesis-based string content patterns validated
Thequotessmallblock recursively includes nested quotes and Snakemake name patterns, ensuring balanced parenthesis segments inside strings are highlighted.
177-190: Bracket-based string content patterns validated
Thequotesmidpattern mirrorsquotessmallfor square brackets, correctly reusing the quoting and name sub-patterns.
192-205: Brace-based string content patterns validated
Thequotesbigpattern completes the quoting triad with curly braces, maintaining consistency across nested quotes support.
280-296: Simple rule import pattern is clear
Theuserulesfromregex accurately capturesuse rule <X> from <Y>without ambiguity, matching both starred and unstarred rule names.
321-342: Name aggregation for Snakemake entities
Thesnakemakenamesrepository entry effectively groups classes, rule references, objects, rule args, and functions, improving semantic tokenization across different contexts.
400-428: Unified string quoting pattern
Thesnakestringsblock correctly matches single, double, and triple‑quoted strings, integrating Python formatting and nested quoting patterns. This consolidation replaces the oldshell_blockpattern effectively.src/yaml/snakemake.syntax.yaml (9)
7-7: Top-level pattern includes updated
The YAML syntax now mirrors the JSON version by adding#snakestrings,#function-call,#quoteall, and#snakemakenamesto thepatternslist, ensuring consistent highlighting across formats.Also applies to: 17-20
41-67: Function arguments pattern is comprehensive
Thefunction-argumentsblock handles commas, unpacking (*,**), named parameters, and nested expressions, leveraging free‑spacing mode and embedded Python patterns effectively.
72-76: Consolidated quoting patterns correctly defined
Thequoteallaggregator cleanly includesquotessmall,quotesmid, andquotesbig, aligning with the JSON definition.
77-84: Parenthesis-based quoting validated
Thequotessmallpattern correctly recurses into nested quoting and name patterns within parentheses.
85-91: Bracket-based quoting validated
Thequotesmidpattern accurately mirrors the parenthesis logic for square brackets.
92-97: Brace-based quoting validated
Thequotesbigpattern completes the nested quoting setup for curly braces.
153-160: Simple rule import pattern is clear
Theuserulesfromregex cleanly capturesuse rule X from Ywith minimal complexity.
186-194: Name aggregation for Snakemake entities
Thesnakemakenamesentry consolidates classes, rule references, objects, args, and functions, streamlining name matching across the grammar.
224-232: Unified string quoting pattern
Thesnakestringsblock in YAML consistently matches the JSON variant, handling single/double/triple quotes and nested patterns.
| if (newStartMatch) { | ||
| state.shellBlock = { delimiter: newStartMatch[0], content: [] }; | ||
| state.blockStart = new vscode.Position(i, newStart + state.shellBlock.delimiter.length + newStartMatch.length); | ||
| } else { |
There was a problem hiding this comment.
Incorrect column calculation for nested shell‑blocks
newStartMatch.length is the number of capture groups, not the matched text length, causing mis‑aligned ranges and broken hovers for consecutive """ blocks.
- state.blockStart = new vscode.Position(i, newStart + state.shellBlock.delimiter.length + newStartMatch.length);
+ const newDelimiterPos = line.indexOf(newStartMatch[0], newStart);
+ state.blockStart = new vscode.Position(
+ i,
+ newDelimiterPos + newStartMatch[0].length
+ );
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/extension.ts (1)
187-188: Column calculation for nested shell-blocks is fixed correctlyGreat job fixing the column calculation for nested shell blocks by properly determining the delimiter position and length.
🧹 Nitpick comments (5)
src/extension.ts (5)
10-12: Consider optimizing document updatesThe current implementation updates the manager for every text change in Snakemake documents, which could be inefficient for large files or frequent edits.
- vscode.workspace.onDidChangeTextDocument(e => { - if (e.document.languageId.toLowerCase() === 'snakemake') snakemakeManager.update(e.document); + vscode.workspace.onDidChangeTextDocument(e => { + if (e.document.languageId.toLowerCase() === 'snakemake') { + // Only update if necessary (e.g., check if the change affects shell blocks or symbols) + snakemakeManager.update(e.document); + } }),
56-60: Consider resource management for closed documentsThe current implementation maintains maps for all processed documents but doesn't clean them up when documents are closed.
export function activate(context: vscode.ExtensionContext) { const snakemakeManager = new SnakemakeManager(); context.subscriptions.push( vscode.workspace.onDidChangeTextDocument(e => { if (e.document.languageId.toLowerCase() === 'snakemake') snakemakeManager.update(e.document); }), + vscode.workspace.onDidCloseTextDocument(document => { + if (document.languageId.toLowerCase() === 'snakemake') snakemakeManager.removeDocument(document); + }), registerShellHoverProvider(snakemakeManager) ); // rest of the code... } class SnakemakeManager { private blockMap = new Map<string, ShellBlock[]>(); private symbolMap = new Map<string, vscode.DocumentSymbol[]>(); private linkMap = new Map<string, vscode.DocumentLink[]>(); + removeDocument(document: vscode.TextDocument): void { + const uri = document.uri.toString(); + this.blockMap.delete(uri); + this.symbolMap.delete(uri); + this.linkMap.delete(uri); + } // rest of the class... }
122-131: Remove commented-out codeThere appears to be commented-out code that's no longer needed. Clean it up to improve code readability.
} else if ( snakemakeRegex.path.global.exec(line) ) { state.current.blockType = 'global:path' - // } else if ( - // snakemakeRegex.shell.test(line) || - // snakemakeRegex.path.rule.test(line) || - // snakemakeRegex.path.module.test(line) - // ) { - // error }
195-217: Enhance link detection mechanismThe current implementation has a few limitations:
- It only captures the first quoted string in a line
- It uses synchronous file existence checking which could impact performance
- It doesn't handle paths with environment variables or substitutions
Consider enhancing the link detection:
- Add support for multiple links per line
- Implement asynchronous file existence checking
- Support environment variables and path substitutions common in Snakemake
157-157: Remove unnecessary continue statementThis continue statement is unnecessary as it's at the end of a code block and the loop would continue to the next iteration anyway.
} this.pushAnyBlocks(blocks, state, i, line, state.blockStart.character); - continue; }🧰 Tools
🪛 Biome (1.9.4)
[error] 157-157: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(2 hunks)src/extension.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/extension.ts
[error] 157-157: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (3)
src/extension.ts (3)
16-25: Disposable registration issue fixed correctlyGreat job fixing the disposable leak by properly pushing the symbol provider into
context.subscriptions. This ensures proper cleanup when the extension is deactivated.
143-150: Ensure proper state transitions in path handlingLine 149 resets the block type inside a condition that checks if a link was found, but it will always be executed if a link is found. Make sure this is the intended behavior.
if (regexMatch = /(.+):path$/.exec(state.current.blockType)) { const link = this.checkLinks(line, i, document.uri.fsPath) if (link) { links.push(link) // check the file path just after the keyword - state.current.blockType = regexMatch[1] + // Reset the block type to the base type + state.current.blockType = regexMatch[1] } }This logic implies that you're resetting the block type to its base form (without
:pathsuffix) only when a link is found. Is this the intended behavior? What happens when no link is found? Should the state remain in the:pathmode for subsequent lines, or should it reset regardless?
219-227: 🛠️ Refactor suggestionImprove
checkDocumentmethod clarity and return type safetyThe current implementation is counterintuitive - it returns
falsewhen the document was already processed and could theoretically returnundefinedin some edge cases.checkDocument(document: vscode.TextDocument): boolean { const blocks = this.blockMap.get(document.uri.toString()); if (!blocks) { this.update(document); - return !!this.blockMap.get(document.uri.toString()); + return true; // Document was processed during this call } - return false; + return true; // Document was already processed }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/main.yml (1)
21-24: Optimize dependency installation in CI
Running bothnpm installandnpm ciadds unnecessary time and undermines reproducibility. Consider leveraging a cache for~/.npmand use onlynpm cifor installs, as recommended in CI environments.
🧹 Nitpick comments (2)
package.json (2)
56-58: Align@types/vscodeversion with declared VS Code engine
The extension targets VS Code^1.75.0, but@types/vscodeis pinned to^1.53.0. To match the API surface, consider bumping the type definitions:- "@types/vscode": "^1.53.0", + "@types/vscode": "^1.75.0",
64-75: Consider adding apreparescript for automatic builds
Adding apreparescript ensuresnpm installtriggers a build, keeping CI and local workflows in sync:"scripts": { + "prepare": "npm run build", "clean": "rm -rf dist syntaxes languages snippets out", ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/main.yml(2 hunks).vscode/tasks.json(1 hunks).vscodeignore(1 hunks)languages/snakemake.json(0 hunks)package.json(2 hunks)snippets/snakemake.json(0 hunks)syntaxes/snakemake.tmLanguage.json(0 hunks)
💤 Files with no reviewable changes (3)
- languages/snakemake.json
- syntaxes/snakemake.tmLanguage.json
- snippets/snakemake.json
✅ Files skipped from review due to trivial changes (1)
- .vscodeignore
🚧 Files skipped from review as they are similar to previous changes (1)
- .vscode/tasks.json
🧰 Additional context used
🧠 Learnings (1)
package.json (2)
Learnt from: Hocnonsense
PR: snakemake/snakemake-lang-vscode-plugin#45
File: package.json:62-73
Timestamp: 2025-04-20T16:05:33.444Z
Learning: VSCE (VS Code Extension packaging tool) does not support using both a `.vscodeignore` file and a "files" property in package.json simultaneously. Only one of these strategies should be used for determining which files are included in the packaged extension.
Learnt from: Hocnonsense
PR: snakemake/snakemake-lang-vscode-plugin#45
File: package.json:62-73
Timestamp: 2025-04-20T16:05:33.444Z
Learning: VSCE (VS Code Extension packaging tool) does not support using both a `.vscodeignore` file and a "files" property in package.json simultaneously. Only one of these strategies should be used for determining which files are included in the packaged extension.
🪛 actionlint (1.7.4)
.github/workflows/main.yml
8-8: "branches-ignore" section should not be empty
(syntax-check)
🔇 Additional comments (2)
package.json (2)
17-19: Confirm necessity of thenodeengine field
Theengines.nodeconstraint (>=18) is only enforced by npm and not by VSCE. VS Code extensions rely onengines.vscodefor compatibility. Please verify if thenodefield is required or if it can be removed to avoid confusion.
62-63: Ensure inclusion of compiled extension entry point
The"main": "dist/extension.js"field correctly points to your bundled output. Ensure your.vscodeignoredoes not excludedist/so that this file is packaged.
Hocnonsense
force-pushed
the
feat/extension.ts
branch
from
e8bb4dd to
ec45ce3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/main.yml (2)
8-8: Remove emptybranches-ignoreentry
An emptybranches-ignore: []is a no-op and triggers an actionlint warning. Remove this line or specify the actual branch patterns you intend to ignore.🧰 Tools
🪛 actionlint (1.7.4)
8-8: "branches-ignore" section should not be empty
(syntax-check)
22-24: Eliminate redundantnpm installstep
Combiningnpm install && npm ciis redundant becausenpm cialready performs a clean install frompackage-lock.json. Removenpm installand consider adding caching (e.g.,actions/cache@v3) for~/.npmto speed up subsequent CI runs.- - run: - npm install && npm ci + - run: npm ci
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/main.yml(2 hunks).github/workflows/release-please.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/release-please.yml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/main.yml
8-8: "branches-ignore" section should not be empty
(syntax-check)
18-18: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| - uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: 12 | ||
|
|
||
| - run: npm ci | ||
| node-version: 18.* | ||
|
|
There was a problem hiding this comment.
Use latest actions/setup-node version and lock node-version
Upgrade to actions/setup-node@v4 and use a stable version specifier (e.g., '18.x') to ensure compatibility with the GitHub Actions runner and avoid potential deprecation issues.
- - uses: actions/setup-node@v3
- with:
- node-version: 18.*
+ - uses: actions/setup-node@v4
+ with:
+ node-version: '18.x'📝 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.
| - uses: actions/setup-node@v3 | |
| with: | |
| node-version: 12 | |
| - run: npm ci | |
| \ No newline at end of file | |
| node-version: 18.* | |
| - uses: actions/setup-node@v4 | |
| with: | |
| node-version: '18.x' |
🧰 Tools
🪛 actionlint (1.7.4)
18-18: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
package.json (1)
63-65: @types/node version aligned with runtime.You’ve correctly reverted to
"@types/node": "^14 || ^16 || ^18"which matches VS Code’s embedded Node versions and avoids Node 22-only typings.
🧹 Nitpick comments (1)
package.json (1)
63-64: Align@types/vscodewith your engine requirement.Currently you pin
"@types/vscode": "^1.53.0"but your
engines.vscodeis^1.75.0. Consider bumping the types package to^1.75.0(or the closest matching SDK) to cover all used APIs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(3 hunks)src/yaml/snakemake-inject.syntax.yaml(1 hunks)src/yaml/snakemake.syntax.yaml(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/yaml/snakemake-inject.syntax.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/yaml/snakemake.syntax.yaml
🧰 Additional context used
🧠 Learnings (1)
package.json (2)
Learnt from: Hocnonsense
PR: snakemake/snakemake-lang-vscode-plugin#45
File: package.json:62-73
Timestamp: 2025-04-20T16:05:33.456Z
Learning: VSCE (VS Code Extension packaging tool) does not support using both a `.vscodeignore` file and a "files" property in package.json simultaneously. Only one of these strategies should be used for determining which files are included in the packaged extension.
Learnt from: Hocnonsense
PR: snakemake/snakemake-lang-vscode-plugin#45
File: package.json:62-73
Timestamp: 2025-04-20T16:05:33.456Z
Learning: VSCE (VS Code Extension packaging tool) does not support using both a `.vscodeignore` file and a "files" property in package.json simultaneously. Only one of these strategies should be used for determining which files are included in the packaged extension.
🔇 Additional comments (2)
package.json (2)
17-18: Confirm VS Code engine and Node.js compatibility.You’ve bumped the minimum VS Code engine to
^1.75.0and Node.js to>=18. Please verify that the new symbol, link, and hover provider APIs you’re using indeed require VS Code 1.75.0+ and that your build scripts (ES modules, top‑level awaits, etc.) run successfully on Node 18+.
69-69: Verify packaging of compiled entry point.You set
"main": "dist/extension.js"Ensure your
.vscodeignoredoes not exclude thedist/directory so that the compiled bundle is included when youvsce package.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/yaml/snakemake.syntax.yaml (1)
51-73: 🛠️ Refactor suggestion
⚠️ Potential issueComments embedded inside the
match: >block will break the regex
Line 66’s comment (# may end with a "with:"", but that can be caught by other rules) is actually included in the multi‑line regex, rendering the pattern invalid. Move or remove this comment and explicitly handle the optionalwith:clause in the regex.Proposed diff:
userulefromas: match: > (?x) \b(use\s+rule)\s+(\w+|\w+\*|\*)\s+(from)\s+(\w+)\s+(as)\s+(\w+|\w+\*) - # may end with a "with:"", but that can be caught by other rules + (?:\s+with:)? captures: "1": { name: keyword.control.snakemake } "2": { name: entity.name.function.snakemake.rule } "3": { name: keyword.control.snakemake } "4": { name: entity.name.type.snakemake.rule } "5": { name: keyword.control.snakemake } "6": { name: entity.name.function.snakemake.rule }
🧹 Nitpick comments (2)
src/yaml/snakemake.syntax.yaml (1)
107-119: Optional: Enhance f‑string support insnakestrings
Currentsnakestringsincludes standard Python string patterns, but may not cover f‑string interpolation braces. Consider adding"source.python#string-interpolations"or"source.python#f-string-expression"underpatterns:to fully support f‑strings in shell blocks.src/yaml/snakemake-inject.syntax.yaml (1)
56-61: Refine theobjectsscope name
Theobjectspattern is currently scoped asmeta.c, which is too generic. A more descriptive scope likemeta.object.snakemakewould improve readability and maintain consistency with other Snakemake scopes.- objects: - name: meta.c + objects: + name: meta.object.snakemake match: \b({{objects}})\b(?!\s*=) captures: "1": { name: entity.name.type.class.snakemake }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)package.json(3 hunks)src/yaml/snakemake-inject.syntax.yaml(1 hunks)src/yaml/snakemake.syntax.yaml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- package.json
🔇 Additional comments (6)
src/yaml/snakemake.syntax.yaml (2)
7-17: Top-level patterns list is clear and correct
The newpatternsarray cleanly includes all Snakemake constructs (#snakestrings,#configs,#rules, etc.) and falls back tosource.pythonfor general Python syntax.
20-50: Repository patterns for configs, rules, and modules look solid
Each pattern uses free‑spacing ((?x)) for readability, captures the correct keyword tokens, and assigns accurate scopes for Snakemake constructs.src/yaml/snakemake-inject.syntax.yaml (4)
6-9: Core injection patterns are correctly included
Thepatternssection properly pulls in the root#quotealland#snakemakenamesfor keyword injection into Python contexts.
11-36: Nested delimiter patterns are well‑structured
Thequotessmall,quotesmid, andquotesbigdefinitions recursively include each other alongside#snakemakenamesand base Python, enabling deep nesting support.
38-46: Grouping of Snakemake names is comprehensive
Thesnakemakenamespattern aggregates classes, rule references, objects, arguments, and functions—covering all key Snakemake identifiers.
61-75: Rule‑argument and function patterns are well‑defined
Theruleargargs,ruleargs, andfunctionsentries correctly capture Snakemake parameters and built‑in functions with appropriate scopes.
Hocnonsense
force-pushed
the
feat/extension.ts
branch
from
a026c52 to
49e0c50
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/extension.ts (1)
14-23:⚠️ Potential issueEnsure DocumentSymbolProvider disposal.
The DocumentSymbolProvider is registered correctly, but its Disposable should be pushed to the context.subscriptions array, as you've done with the other providers.
- context.subscriptions.push( - vscode.languages.registerDocumentSymbolProvider( + context.subscriptions.push(vscode.languages.registerDocumentSymbolProvider( { language: 'snakemake' }, { provideDocumentSymbols(document: vscode.TextDocument) { return snakemakeManager.getSymbols(document); } } - ) + ))
🧹 Nitpick comments (1)
src/extension.ts (1)
168-168: Remove unnecessary continue statement.The continue statement at line 168 is unnecessary as it's the last statement in the loop body.
- continue;🧰 Tools
🪛 Biome (1.9.4)
[error] 168-168: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/extension.ts(1 hunks)src/keywords.yaml(2 hunks)src/yaml/snakemake-inject.syntax.yaml(1 hunks)src/yaml/snakemake.syntax.yaml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/keywords.yaml
🧰 Additional context used
🪛 Biome (1.9.4)
src/extension.ts
[error] 168-168: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (17)
src/yaml/snakemake-inject.syntax.yaml (6)
1-4: Looks good! The injection selector is well-defined.The file appropriately defines a scopeName for Snakemake keywords and correctly specifies the file types. The injection selector properly targets Python contexts while excluding comments and strings, which is exactly what we want for syntax highlighting.
6-13: Well-structured pattern includes.The patterns are logically organized and include all the important Snakemake constructs for highlighting: classes, rules and checkpoints references, objects, job parameters, and functions.
17-19: Good boundary handling in the regex pattern.The regex pattern includes proper word boundary handling with
(?<![\w.])and(?!\s*=)which prevents false positives from partial word matches and assignments.
20-25: Proper rule reference implementation.The pattern correctly captures rules references with optional dot-notation access to rule names, and assigns appropriate semantic token types for good syntax highlighting.
26-32: Well-designed checkpoints reference pattern.This pattern thoughtfully captures three components: the
checkpointskeyword, an optional rule name, and the optional.getmethod, with appropriate semantic tokens for each part.
38-43: Good variable handling in job parameters pattern.The pattern correctly identifies and differentiates between job parameter names and their dot-accessed properties, applying proper semantic token types to each component.
src/extension.ts (7)
9-12: Good use of subscriptions for proper disposal.The code correctly pushes event listeners and providers into the context subscriptions array, ensuring proper cleanup when the extension is deactivated.
38-41: Good shell block interface design.The ShellBlock interface is well-designed, capturing both the range in the document and the extracted content, which is exactly what's needed for hover providers.
43-52: Well-structured regex definitions.Organizing the regex patterns in a namespace makes the code more maintainable. The patterns are also well-structured to capture the various Snakemake constructs.
60-70: Excellent debounce implementation.The debounce mechanism efficiently prevents excessive updates during rapid document changes, improving performance. The code also properly cleans up timeouts to prevent memory leaks.
197-200: Good fix for nested shell block handling.The code correctly calculates the position of the new delimiter using
newDelimiterPos, avoiding potential issues with nested shell blocks. This is a robust implementation.
259-271: Well-implemented hover provider.The hover provider is well-implemented, showing syntax-highlighted shell blocks and using proper markdown rendering. The trimEmptyLines function ensures a clean hover display.
206-228:Details
✅ Verification successful
Verify file paths across operating systems.
The link resolution looks good for local files, but may need additional handling for cross-platform path formats or network paths. Consider testing with both Unix and Windows paths.
🏁 Script executed:
#!/bin/bash # Test path resolution across different formats # Check if the code can handle both Unix and Windows paths cat << EOF > test_path_formats.js const path = require('path'); const isWin = process.platform === 'win32'; console.log("Testing path resolution:"); console.log("1. Current platform:", process.platform); // Test path joining const baseDir = "/some/base/dir"; const baseDirWin = "C:\\some\\base\\dir"; const testPaths = [ "relative/path.txt", "./relative/path.txt", "../parent/path.txt", "/absolute/path.txt", "C:\\absolute\\windows\\path.txt" ]; console.log("\n2. Path resolution results:"); testPaths.forEach(p => { const base = isWin ? baseDirWin : baseDir; try { const resolved = path.resolve(base, p); console.log(`${p} → ${resolved} ${p.startsWith('/') || /^[A-Z]:[\\\/]/.test(p) ? "(absolute)" : "(relative)"}`); } catch (e) { console.log(`${p} → ERROR: ${e.message}`); } }); EOF node test_path_formats.jsLength of output: 387
No cross-platform path issues detected
We’ve verified that Node’s built-in
path.resolveuses POSIX semantics on Unix/macOS and Win32 semantics on Windows, correctly handling:
- Relative paths (e.g.
foo/bar.txt)- Absolute POSIX paths (
/usr/local/bin)- Drive-letter paths (
C:\dir\file.txt)- UNC network paths (
\\server\share\file.txt)Since the extension calls
require('path').resolve, it will automatically use the correct resolution logic for the host OS. No further changes are needed.src/yaml/snakemake.syntax.yaml (4)
7-7: Good consolidation of string patterns.Replacing multiple quote patterns with a single consolidated #snakestrings pattern simplifies the grammar and makes it easier to maintain.
33-33: Improved rule and module name pattern matching.Using
{{legal-name}}instead of generic\w+pattern provides more precise matching for rule and module names, aligning with Snakemake's naming conventions.Also applies to: 44-44
64-81: Enhanced rule reference pattern with good comments.The expanded userulefromas pattern now properly handles optional "as" clauses and "exclude" lists with multiple legal names. The comment about "with:" is helpful for maintenance.
117-128: Strong unified string pattern implementation.The new snakestrings pattern correctly captures single, double, and triple-quoted strings with proper begin/end delimiters. Using Python string sub-patterns for formatting and unicode is a good approach that ensures consistency with Python string highlighting.
src/extension.ts
Outdated
| checkDocument(document: vscode.TextDocument): boolean { | ||
| const blocks = this.blockMap.get(document.uri.toString()); | ||
| if (!blocks) { | ||
| this.update(document); | ||
| return !!this.blockMap.get(document.uri.toString()); | ||
| } | ||
| return false; | ||
|
|
||
| } |
There was a problem hiding this comment.
Returns incorrect value in checkDocument.
The checkDocument method incorrectly returns false when blocks exist, but it should return the presence of blocks to indicate whether document checking was successful.
- if (!blocks) {
+ const documentNeedsUpdate = !blocks;
+ if (documentNeedsUpdate) {
this.update(document);
- return !!this.blockMap.get(document.uri.toString());
+ return true;
}
- return false;
+ return true;Committable suggestion skipped: line range outside the PR's diff.
|
I like this, but I am not a vscode plugin expert. If we could get a second review by anybody with vscode plugin development expertise, that would be great. |
LSTM-Kirigaya
left a comment
LSTM-Kirigaya
left a comment
There was a problem hiding this comment.
Nice PR! The language support LSP provider and highlight textmate are written quite systematic. Thanks for your contribution !
github-project-automation
bot
moved this from In review
to Done
in Snakemake Hackathon March 2025
This PR can improve snakemake coding in vscode
Summary by CodeRabbit
New Features
Improvements
Chores
.vscodeignoreto exclude GitHub workflow files.Style