feat: Add approval evidence to sprint dry-runs#628
Conversation
![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 implements shared approval evidence for sprint-related dry-run mutations, including planning, starting, closing, and rolling over sprints. It introduces schema versioning for sprint reports and updates the domain layer to automatically populate approval evidence. The review feedback highlights that the manual schema and approval assignments added to the CLI layer are redundant as the domain functions already handle these tasks. Additionally, there is a recommendation to maintain internal consistency by using the existing shell-quoting utility function within the approval logic.
| gira.EnsureSprintPlanReportSchema(&report) | ||
| if report.Mode == "dry-run" { | ||
| report.Approval = gira.SprintPlanApprovalEvidence(report) | ||
| } |
There was a problem hiding this comment.
This logic is redundant. The gira.PlanSprint function (called at line 7454) already populates the SchemaVersion and Approval fields when in dry-run mode. Re-assigning them here in the CLI layer is unnecessary and can be removed to simplify the code.
| gira.EnsureSprintStartReportSchema(&report) | ||
| if report.Mode == "dry-run" { | ||
| report.Approval = gira.SprintStartApprovalEvidence(report) | ||
| } |
There was a problem hiding this comment.
Redundant logic. gira.StartSprint already ensures the schema version and populates the approval evidence for dry-runs. These lines should be removed to keep the CLI layer clean and avoid duplicate processing.
| gira.EnsureSprintCloseReportSchema(&report) | ||
| if report.Mode == "dry-run" { | ||
| report.Approval = gira.SprintCloseApprovalEvidence(report) | ||
| } |
There was a problem hiding this comment.
Redundant logic. gira.CloseSprint already handles schema versioning and approval evidence population for dry-runs. This duplication in the CLI layer is unnecessary.
| gira.EnsureSprintRolloverReportSchema(&report) | ||
| if report.Mode == "dry-run" { | ||
| report.Approval = gira.SprintRolloverApprovalEvidence(report) | ||
| } |
There was a problem hiding this comment.
Redundant logic. The domain functions called by newSprintRolloverReport already populate these fields. This assignment in the CLI layer is unnecessary and should be removed.
| "--repo", QuoteShellArg(report.Repo), | ||
| "--iteration", QuoteShellArg(report.Iteration), |
There was a problem hiding this comment.
Inconsistent quoting function usage. The existing code in this file uses the private shellQuoteArg (defined at line 185), while the new sprint-related functions use QuoteShellArg. For consistency within the file, consider using shellQuoteArg throughout, or unify the quoting logic across the package if QuoteShellArg is intended to be the new standard.
References
- Maintain internal consistency in utility function usage within the same file. (link)
1 participant
Closes #627