Repository navigation
feat(task): skip dependencies with empty templated names - #11057
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDependency rendering now omits templated task names that render empty, defers usage-dependent templates until usage values exist, and preserves literal empty names as invalid. Unit and end-to-end tests cover both behaviors. ChangesDependency rendering behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Greptile SummaryThis PR adds filtering of dependency entries (
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to filtering empty-rendering templated deps and does not alter any task execution or scheduling logic beyond what is explicitly tested. The two-phase rendering contract (initial non-usage pass → deferred usage-value pass from raw) is preserved correctly. Literal empty names still flow through to the existing 'task not found' validation. The std::mem::take refactor eliminates all six former loops without changing observable behavior for non-empty deps. Unit tests cover all four branching outcomes of render(), and the e2e test exercises depends, depends_post, wait_for, ordering, and the error path. No files require special attention. Important Files Changed
Reviews (6): Last reviewed commit: "fix(task): satisfy clippy in dependency ..." | Re-trigger Greptile |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/task/task_dep.rs (1)
23-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider trimming the rendered task string before checking if it's empty.
If a templated task includes accidental whitespace (e.g.,
" {% if usage.enabled %}dependency{% endif %} "), it will evaluate to a string containing only spaces when the condition evaluates to false. Checkingself.task.trim().is_empty()ensures these are still properly skipped instead of causing atask not found:error down the line.♻️ Proposed fix
) -> crate::Result<bool> { let task_is_templated = contains_template_syntax(&self.task); if task_is_templated { self.task = render_str(tera, &self.task, tera_ctx)?; - if self.task.is_empty() { + if self.task.trim().is_empty() { return Ok(false); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/task/task_dep.rs` around lines 23 - 29, Update the rendered-task emptiness check in the templated-task branch of the task dependency method to use trimmed content, so whitespace-only results return Ok(false) and do not proceed to task lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/task/task_dep.rs`:
- Around line 23-29: Update the rendered-task emptiness check in the
templated-task branch of the task dependency method to use trimmed content, so
whitespace-only results return Ok(false) and do not proceed to task lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 8037ccb6-850a-4bed-bdbd-402ff3b0b8fe
📒 Files selected for processing (3)
e2e/tasks/test_task_dep_argssrc/task/mod.rssrc/task/task_dep.rs
d331957 to
e9d3541
Compare
e9d3541 to
c59f54b
Compare
c59f54b to
59cd0b6
Compare
59cd0b6 to
30246f4
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 3 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Summary
depends,depends_post, andwait_forentries when a templated task name renders empty or whitespace-onlyusage.*renderingRoot cause
TaskDep::renderrendered dependency task names in place, but callers always retained the dependency. A conditional name that rendered to an empty string therefore reached task matching and failed withtask not found:.The renderer now reports whether the dependency should be retained, and the shared list helper filters only names that contained template syntax and rendered empty. Literal empty names remain in the list and continue through the existing validation path.
Real-world example
Biwa previously needed a hidden no-op task to make schema generation conditional in CI:
With this change, the dependency itself can be omitted:
When
CIis defined, the rendered task name is empty and skipped. Outside CI,render:schemastill runs first. risu729/biwa#868 removes the workaround using this behavior.User impact
Tasks can use Tera conditions to make complete dependency entries optional, matching the conditional structured-argument behavior added in #11055.
Validation
mise run formatcargo test task_dep::testsmise run test:e2e test_task_dep_argsSummary by CodeRabbit
depends,depends_post, andwait_forthat resolve to an empty task name from templates are now omitted.wait_forexecution order.TaskDeprendering, including empty/whitespace-only behavior.