Fix wails3 task variable injection#5135
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughEnsures CLI Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.17.0)v3/internal/commands/task.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m 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.
🧹 Nitpick comments (1)
v3/internal/commands/task.go (1)
175-186: Consider centralizing CLI var propagation to reduce drift risk.The dual writes at Line 179 and Line 183 are correct, but this parsing behavior is mirrored elsewhere (e.g.,
parseTaskCallin tests). A small local helper here would make future updates safer.Refactor sketch
+ setParsedVar := func(key, value string) { + call.Vars.Set(key, ast.Var{Value: value}) + if e.Taskfile != nil { + e.Taskfile.Vars.Set(key, ast.Var{Value: value}) + } + } + for _, v := range cliVars { if strings.Contains(v, "=") { parts := strings.SplitN(v, "=", 2) if len(parts) == 2 { - call.Vars.Set(parts[0], ast.Var{ - Value: parts[1], - }) - if e.Taskfile != nil { - e.Taskfile.Vars.Set(parts[0], ast.Var{ - Value: parts[1], - }) - } + setParsedVar(parts[0], parts[1]) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/internal/commands/task.go` around lines 175 - 186, Centralize the CLI var parsing/propagation by extracting the repeated logic in the loop over cliVars into a small helper (e.g., parseAndPropagateCLIVar or setCLIVar) that takes the raw var string (or key/value), the Call object (call.Vars.Set) and the executor/context (e.Taskfile) and performs the split-on-"=" check and the two Set calls; replace the inline block in the cliVars loop with a call to this helper and update parseTaskCall test usages to call the same helper or mirror its behavior so parsing logic is single-sourced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v3/internal/commands/task.go`:
- Around line 175-186: Centralize the CLI var parsing/propagation by extracting
the repeated logic in the loop over cliVars into a small helper (e.g.,
parseAndPropagateCLIVar or setCLIVar) that takes the raw var string (or
key/value), the Call object (call.Vars.Set) and the executor/context
(e.Taskfile) and performs the split-on-"=" check and the two Set calls; replace
the inline block in the cliVars loop with a call to this helper and update
parseTaskCall test usages to call the same helper or mirror its behavior so
parsing logic is single-sourced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b7201ae7-c1f7-4f1d-9ec0-4f03d2ac59b0
📒 Files selected for processing (1)
v3/internal/commands/task.go
|
This looks good to me can you add the change to |
… of `e.Taskfile.Vars` to make sure variables are injected earlier in the lifecycle
|
Changelog has been updated |
|
✅ Triaged by Wails PR Reviewer This PR has been reviewed and accepted. Test sub-issues have been created for all platforms. Head Ref OID: This comment serves as a signature that this PR has been triaged. Future runs will skip this PR based on the headRefOid. |
|
Tested on Windows — verdict: ✓ pass
The |
|
The CI "failure" on this run is a cancellation, not a test failure. Run breakdown (run 24403404466):
The macOS job checked out the code successfully but was cancelled during For reference, the previous commit on this branch ( |
3 participants
Description
call.Varsis not visible at the top level so, also make use ofe.Taskfile.Varsto make sure variables are injected earlier in the lifecycleThere is a bug where variables were not properly being forwarded from
wails3 task ...This PR is additive to also make use of a variable declaration section that is visible to the whole Taskfile
Fixes # (issue)
Type of change
Please select the option that is relevant.
How Has This Been Tested?
The fastest way to test this bug (without even needing to modify wails at all) is:
Step 1
build/darwin/Taskfile.yml)DEBUG_MODE: '{{default "false" .DEBUG_MODE}}'.runtask withStep 2
atterpac/refreshconfig for your project and modify the execution list (comment out tasks unimportant for the test) and add the debug mode property as shown belowStep 3
Run
wails3 devSummary by CodeRabbit