Fix three critical UX issues in opal-task
Issue 1: Fix recurrence calculation for overdue tasks - Use completion date (End) as base for next instance, not original due date - If task due Monday completed Wednesday, next is Wednesday+7d not Monday+7d - Fallback to Due date if End is not set - Update test to verify new behavior Issue 2: Fix description parsing to work without quotes - Add parseAddArgs() to extract description from non-modifier words - Description = all words that don't start with +, -, or contain : - Enables: opal add buy groceries +shop carrots → 'buy groceries carrots' - Validate description is required (error if only modifiers) - Validate recurring tasks require due date Issue 3: Implement flexible command syntax - Add preprocessArgs() to parse arguments before Cobra routing - Detect command position and split filters (left) from modifiers (right) - Rewrite os.Args so Cobra routes correctly - Enable both 'opal 2 done' and 'opal done 2' syntax - Commands without modifiers accept filters on either side - Commands with modifiers enforce [filters] command [modifiers] - Add confirmation for modify without filters (modifies all tasks) All commands updated to use preprocessed ParsedArgs from context. All tests passing (33 tests).
This commit is contained in:
+47
-9
@@ -15,12 +15,15 @@ var addCmd = &cobra.Command{
|
||||
Long: `Add a new task with optional modifiers.
|
||||
|
||||
Examples:
|
||||
opal add "Buy groceries"
|
||||
opal add "Review PR" priority:H project:backend
|
||||
opal add "Team meeting" due:mon recur:1w +meetings`,
|
||||
Args: cobra.MinimumNArgs(1),
|
||||
opal add buy groceries # No quotes needed!
|
||||
opal add review PR priority:H project:backend
|
||||
opal add buy groceries +shop carrots # Tag can be anywhere
|
||||
opal add team meeting due:mon recur:1w +meetings`,
|
||||
Run: func(cmd *cobra.Command, args []string) {
|
||||
if err := addTask(args); err != nil {
|
||||
parsed := getParsedArgs(cmd)
|
||||
// For add, combine filters and modifiers (all are args to parse)
|
||||
allArgs := append(parsed.Filters, parsed.Modifiers...)
|
||||
if err := addTask(allArgs); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
@@ -28,13 +31,16 @@ Examples:
|
||||
}
|
||||
|
||||
func addTask(args []string) error {
|
||||
// First arg is description, rest are modifiers
|
||||
description := args[0]
|
||||
modifierArgs := args[1:]
|
||||
// Parse description and modifiers from args
|
||||
// Description = all words that are NOT filters/modifiers
|
||||
// Filters/Modifiers = words with +, -, or containing :
|
||||
description, modifierArgs, err := parseAddArgs(args)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Parse modifiers
|
||||
var mod *engine.Modifier
|
||||
var err error
|
||||
|
||||
if len(modifierArgs) > 0 {
|
||||
mod, err = engine.ParseModifier(modifierArgs)
|
||||
@@ -65,6 +71,33 @@ func addTask(args []string) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// parseAddArgs extracts description and modifiers from args
|
||||
// Description = all non-filter/modifier words joined with spaces
|
||||
// Filters/Modifiers = args with +, -, or containing :
|
||||
func parseAddArgs(args []string) (string, []string, error) {
|
||||
var descParts []string
|
||||
var modifiers []string
|
||||
|
||||
for _, arg := range args {
|
||||
isFilterOrModifier := strings.HasPrefix(arg, "+") ||
|
||||
strings.HasPrefix(arg, "-") ||
|
||||
strings.Contains(arg, ":")
|
||||
|
||||
if isFilterOrModifier {
|
||||
modifiers = append(modifiers, arg)
|
||||
} else {
|
||||
descParts = append(descParts, arg)
|
||||
}
|
||||
}
|
||||
|
||||
if len(descParts) == 0 {
|
||||
return "", nil, fmt.Errorf("description is required")
|
||||
}
|
||||
|
||||
description := strings.Join(descParts, " ")
|
||||
return description, modifiers, nil
|
||||
}
|
||||
|
||||
func addRecurringTask(description string, mod *engine.Modifier) error {
|
||||
// Extract recurrence pattern
|
||||
recurPattern := mod.SetAttributes["recur"]
|
||||
@@ -72,6 +105,11 @@ func addRecurringTask(description string, mod *engine.Modifier) error {
|
||||
return fmt.Errorf("no recurrence pattern specified")
|
||||
}
|
||||
|
||||
// Validate: recurring tasks must have due date
|
||||
if mod.SetAttributes["due"] == nil {
|
||||
return fmt.Errorf("recurring tasks require a due date (use due:YYYY-MM-DD or due:monday)")
|
||||
}
|
||||
|
||||
duration, err := engine.ParseRecurrencePattern(*recurPattern)
|
||||
if err != nil {
|
||||
return fmt.Errorf("invalid recurrence pattern: %w", err)
|
||||
|
||||
@@ -12,7 +12,8 @@ var countCmd = &cobra.Command{
|
||||
Use: "count [filter...]",
|
||||
Short: "Count matching tasks",
|
||||
Run: func(cmd *cobra.Command, args []string) {
|
||||
if err := countTasks(args); err != nil {
|
||||
parsed := getParsedArgs(cmd)
|
||||
if err := countTasks(parsed.Filters); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
@@ -12,7 +12,8 @@ var deleteCmd = &cobra.Command{
|
||||
Use: "delete [filter...]",
|
||||
Short: "Delete tasks",
|
||||
Run: func(cmd *cobra.Command, args []string) {
|
||||
if err := deleteTasks(args); err != nil {
|
||||
parsed := getParsedArgs(cmd)
|
||||
if err := deleteTasks(parsed.Filters); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
@@ -14,11 +14,14 @@ var doneCmd = &cobra.Command{
|
||||
Long: `Mark one or more tasks as completed.
|
||||
|
||||
Examples:
|
||||
opal 1 done # Complete task with display ID 1
|
||||
opal +urgent done # Complete all urgent tasks
|
||||
opal project:backend done`,
|
||||
opal done 1 # Complete task with display ID 1
|
||||
opal 1 done # Flexible syntax (same as above)
|
||||
opal done +urgent # Complete all urgent tasks
|
||||
opal +urgent done # Flexible syntax (same as above)
|
||||
opal done project:backend`,
|
||||
Run: func(cmd *cobra.Command, args []string) {
|
||||
if err := completeTasks(args); err != nil {
|
||||
parsed := getParsedArgs(cmd)
|
||||
if err := completeTasks(parsed.Filters); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
@@ -17,9 +17,11 @@ Examples:
|
||||
opal list # List all pending tasks
|
||||
opal list +home # List tasks with +home tag
|
||||
opal list project:backend # List backend project tasks
|
||||
opal list priority:H # List high priority tasks`,
|
||||
opal list priority:H # List high priority tasks
|
||||
opal 2 list # List using filter 2 (flexible syntax)`,
|
||||
Run: func(cmd *cobra.Command, args []string) {
|
||||
if err := listTasks(args); err != nil {
|
||||
parsed := getParsedArgs(cmd)
|
||||
if err := listTasks(parsed.Filters); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
+65
-25
@@ -9,64 +9,104 @@ import (
|
||||
)
|
||||
|
||||
var modifyCmd = &cobra.Command{
|
||||
Use: "modify [filter...] [modifiers...]",
|
||||
Use: "modify [modifier...]",
|
||||
Short: "Modify task attributes",
|
||||
Long: `Modify task attributes. Filters must come before 'modify', modifiers after.
|
||||
|
||||
Examples:
|
||||
opal 2 modify priority:H
|
||||
opal +urgent modify due:tomorrow
|
||||
opal project:backend modify priority:L
|
||||
opal modify priority:H # Modify ALL pending tasks (with confirmation)`,
|
||||
Run: func(cmd *cobra.Command, args []string) {
|
||||
if err := modifyTasks(args); err != nil {
|
||||
parsed := getParsedArgs(cmd)
|
||||
if err := modifyTasks(parsed.Filters, parsed.Modifiers); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
},
|
||||
}
|
||||
|
||||
func modifyTasks(args []string) error {
|
||||
// Split into filter and modifier
|
||||
// Simple heuristic: modifiers contain ':', filters might not
|
||||
filterArgs := []string{}
|
||||
modifierArgs := []string{}
|
||||
|
||||
for _, arg := range args {
|
||||
// If it starts with + or -, it could be either
|
||||
// If it contains : and a value, it's likely a modifier
|
||||
// Numeric args are filters
|
||||
// For now, put everything in modifiers (will be improved)
|
||||
modifierArgs = append(modifierArgs, arg)
|
||||
}
|
||||
|
||||
func modifyTasks(filterArgs, modifierArgs []string) error {
|
||||
// Validate we have modifiers
|
||||
if len(modifierArgs) == 0 {
|
||||
return fmt.Errorf("no modifiers specified")
|
||||
return fmt.Errorf("no modifiers specified (modifiers must come after 'modify')")
|
||||
}
|
||||
|
||||
// Try to parse as filter first to get the ID
|
||||
filter, _ := engine.ParseFilter(filterArgs)
|
||||
// Parse filter (or use default if no filters specified)
|
||||
var filter *engine.Filter
|
||||
var err error
|
||||
|
||||
ws, _ := engine.LoadWorkingSet()
|
||||
if len(filterArgs) == 0 {
|
||||
// No filters = modify all pending tasks (with confirmation)
|
||||
filter = engine.DefaultFilter()
|
||||
} else {
|
||||
filter, err = engine.ParseFilter(filterArgs)
|
||||
if err != nil {
|
||||
return fmt.Errorf("failed to parse filter: %w", err)
|
||||
}
|
||||
}
|
||||
|
||||
// Load working set for ID resolution
|
||||
ws, err := engine.LoadWorkingSet()
|
||||
if err != nil {
|
||||
// If no working set exists yet, build one
|
||||
ws, err = engine.BuildWorkingSet(filter)
|
||||
if err != nil {
|
||||
return fmt.Errorf("failed to build working set: %w", err)
|
||||
}
|
||||
}
|
||||
|
||||
// Resolve tasks
|
||||
var tasks []*engine.Task
|
||||
if ws != nil && len(filter.IDs) > 0 {
|
||||
|
||||
if len(filter.IDs) > 0 {
|
||||
// Resolve display IDs
|
||||
for _, id := range filter.IDs {
|
||||
task, err := ws.GetTaskByDisplayID(id)
|
||||
if err == nil {
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
tasks = append(tasks, task)
|
||||
}
|
||||
} else {
|
||||
// Use filter to get tasks
|
||||
tasks, err = engine.GetTasks(filter)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
if len(tasks) == 0 {
|
||||
return fmt.Errorf("no tasks to modify")
|
||||
return fmt.Errorf("no tasks matched")
|
||||
}
|
||||
|
||||
// Confirm if multiple tasks or no filters specified
|
||||
if len(tasks) > 1 || len(filterArgs) == 0 {
|
||||
fmt.Printf("About to modify %d task(s). Proceed? (y/N): ", len(tasks))
|
||||
var confirm string
|
||||
fmt.Scanln(&confirm)
|
||||
if confirm != "y" && confirm != "Y" {
|
||||
fmt.Println("Cancelled.")
|
||||
return nil
|
||||
}
|
||||
}
|
||||
|
||||
// Parse and apply modifiers
|
||||
mod, err := engine.ParseModifier(modifierArgs)
|
||||
if err != nil {
|
||||
return fmt.Errorf("failed to parse modifiers: %w", err)
|
||||
}
|
||||
|
||||
modified := 0
|
||||
for _, task := range tasks {
|
||||
if err := mod.Apply(task); err != nil {
|
||||
return fmt.Errorf("failed to modify task: %w", err)
|
||||
fmt.Fprintf(os.Stderr, "Warning: failed to modify task %s: %v\n", task.UUID, err)
|
||||
} else {
|
||||
modified++
|
||||
}
|
||||
}
|
||||
|
||||
fmt.Printf("Modified %d task(s).\n", len(tasks))
|
||||
fmt.Printf("Modified %d task(s).\n", modified)
|
||||
return nil
|
||||
}
|
||||
|
||||
+121
-2
@@ -1,6 +1,7 @@
|
||||
package cmd
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"os"
|
||||
|
||||
@@ -8,14 +9,38 @@ import (
|
||||
"github.com/spf13/cobra"
|
||||
)
|
||||
|
||||
// ParsedArgs represents preprocessed command arguments
|
||||
type ParsedArgs struct {
|
||||
Command string
|
||||
Filters []string
|
||||
Modifiers []string
|
||||
}
|
||||
|
||||
// Context key for parsed args
|
||||
type contextKey string
|
||||
|
||||
const parsedArgsKey contextKey = "parsedArgs"
|
||||
|
||||
// Command classification
|
||||
var commandNames = []string{
|
||||
"add", "list", "done", "modify", "delete",
|
||||
"start", "stop", "count", "projects", "tags",
|
||||
}
|
||||
|
||||
var commandsWithModifiers = map[string]bool{
|
||||
"add": true,
|
||||
"modify": true,
|
||||
}
|
||||
|
||||
var rootCmd = &cobra.Command{
|
||||
Use: "opal",
|
||||
Short: "Opal task manager - taskwarrior-inspired CLI task management",
|
||||
Long: `Opal is a powerful command-line task manager inspired by taskwarrior.
|
||||
It supports filtering, tags, priorities, projects, and recurring tasks.`,
|
||||
Run: func(cmd *cobra.Command, args []string) {
|
||||
// Default behavior: show pending tasks
|
||||
if err := listTasks(args); err != nil {
|
||||
// Default behavior: list tasks
|
||||
parsed := getParsedArgs(cmd)
|
||||
if err := listTasks(parsed.Filters); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
@@ -23,9 +48,103 @@ It supports filtering, tags, priorities, projects, and recurring tasks.`,
|
||||
}
|
||||
|
||||
func Execute() error {
|
||||
// Preprocess arguments BEFORE Cobra routing
|
||||
if len(os.Args) > 1 {
|
||||
parsed := preprocessArgs(os.Args[1:])
|
||||
|
||||
// Store in context for commands to use
|
||||
ctx := context.WithValue(context.Background(), parsedArgsKey, parsed)
|
||||
rootCmd.SetContext(ctx)
|
||||
|
||||
// Rewrite os.Args for Cobra based on parsed command
|
||||
// This allows Cobra to route to the correct command
|
||||
if parsed.Command != "list" || len(parsed.Filters) > 0 || len(parsed.Modifiers) > 0 {
|
||||
// Reconstruct args: [command, ...filters, ...modifiers]
|
||||
newArgs := []string{os.Args[0], parsed.Command}
|
||||
newArgs = append(newArgs, parsed.Filters...)
|
||||
newArgs = append(newArgs, parsed.Modifiers...)
|
||||
os.Args = newArgs
|
||||
}
|
||||
}
|
||||
|
||||
return rootCmd.Execute()
|
||||
}
|
||||
|
||||
// getParsedArgs retrieves preprocessed args from context
|
||||
func getParsedArgs(cmd *cobra.Command) *ParsedArgs {
|
||||
if v := cmd.Context().Value(parsedArgsKey); v != nil {
|
||||
if parsed, ok := v.(*ParsedArgs); ok {
|
||||
return parsed
|
||||
}
|
||||
}
|
||||
return &ParsedArgs{}
|
||||
}
|
||||
|
||||
// preprocessArgs parses command-line arguments before Cobra routing
|
||||
// Returns: command name, filters, modifiers
|
||||
func preprocessArgs(args []string) *ParsedArgs {
|
||||
if len(args) == 0 {
|
||||
return &ParsedArgs{
|
||||
Command: "list", // Default command
|
||||
Filters: []string{},
|
||||
Modifiers: []string{},
|
||||
}
|
||||
}
|
||||
|
||||
// Find command position
|
||||
cmdIdx := -1
|
||||
cmdName := ""
|
||||
|
||||
for i, arg := range args {
|
||||
for _, name := range commandNames {
|
||||
if arg == name {
|
||||
cmdIdx = i
|
||||
cmdName = name
|
||||
break
|
||||
}
|
||||
}
|
||||
if cmdIdx >= 0 {
|
||||
break
|
||||
}
|
||||
}
|
||||
|
||||
// If no command found, treat as filters for default list command
|
||||
if cmdIdx == -1 {
|
||||
return &ParsedArgs{
|
||||
Command: "list",
|
||||
Filters: args,
|
||||
Modifiers: []string{},
|
||||
}
|
||||
}
|
||||
|
||||
// Split arguments around command
|
||||
leftArgs := args[:cmdIdx] // Everything before command
|
||||
rightArgs := []string{}
|
||||
if cmdIdx+1 < len(args) {
|
||||
rightArgs = args[cmdIdx+1:] // Everything after command
|
||||
}
|
||||
|
||||
// Determine how to interpret right args
|
||||
if commandsWithModifiers[cmdName] {
|
||||
// Command accepts modifiers
|
||||
// Left = filters, Right = modifiers
|
||||
return &ParsedArgs{
|
||||
Command: cmdName,
|
||||
Filters: leftArgs,
|
||||
Modifiers: rightArgs,
|
||||
}
|
||||
} else {
|
||||
// Command doesn't accept modifiers
|
||||
// Both left and right are filters
|
||||
allFilters := append(leftArgs, rightArgs...)
|
||||
return &ParsedArgs{
|
||||
Command: cmdName,
|
||||
Filters: allFilters,
|
||||
Modifiers: []string{},
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func init() {
|
||||
cobra.OnInitialize(initializeApp)
|
||||
|
||||
|
||||
@@ -12,7 +12,8 @@ var startCmd = &cobra.Command{
|
||||
Use: "start [filter...]",
|
||||
Short: "Start a task (set start time)",
|
||||
Run: func(cmd *cobra.Command, args []string) {
|
||||
if err := startTasks(args); err != nil {
|
||||
parsed := getParsedArgs(cmd)
|
||||
if err := startTasks(parsed.Filters); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
@@ -12,7 +12,8 @@ var stopCmd = &cobra.Command{
|
||||
Use: "stop [filter...]",
|
||||
Short: "Stop a task (clear start time)",
|
||||
Run: func(cmd *cobra.Command, args []string) {
|
||||
if err := stopTasks(args); err != nil {
|
||||
parsed := getParsedArgs(cmd)
|
||||
if err := stopTasks(parsed.Filters); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "Error: %v\n", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
@@ -76,12 +76,19 @@ func SpawnNextInstance(completedInstance *Task) error {
|
||||
}
|
||||
|
||||
// Calculate next due date
|
||||
var nextDue *time.Time
|
||||
if completedInstance.Due != nil {
|
||||
next := CalculateNextDue(*completedInstance.Due, *template.RecurrenceDuration)
|
||||
nextDue = &next
|
||||
// Use End date (completion time) as base, fallback to Due date if End not set
|
||||
var baseDate time.Time
|
||||
if completedInstance.End != nil {
|
||||
baseDate = *completedInstance.End
|
||||
} else if completedInstance.Due != nil {
|
||||
baseDate = *completedInstance.Due
|
||||
} else {
|
||||
return fmt.Errorf("recurring instance has no due or end date")
|
||||
}
|
||||
|
||||
next := CalculateNextDue(baseDate, *template.RecurrenceDuration)
|
||||
nextDue := &next
|
||||
|
||||
// Check if we're past 'until' date
|
||||
if template.Until != nil && nextDue != nil && nextDue.After(*template.Until) {
|
||||
// Don't spawn, recurrence has expired
|
||||
|
||||
@@ -209,14 +209,18 @@ func TestSpawnNextInstance(t *testing.T) {
|
||||
found = true
|
||||
|
||||
// Verify due date was advanced
|
||||
// New behavior: calculates from End date (completion time), not original due date
|
||||
if task.Due == nil {
|
||||
t.Error("New instance should have due date")
|
||||
} else {
|
||||
expectedDue := CalculateNextDue(*instance1.Due, duration)
|
||||
// Allow 1 second tolerance due to Unix timestamp precision
|
||||
diff := task.Due.Sub(expectedDue)
|
||||
if diff < -time.Second || diff > time.Second {
|
||||
t.Errorf("Expected due date %v, got %v (diff: %v)", expectedDue, *task.Due, diff)
|
||||
// Should be 7 days from when we completed instance1
|
||||
// Allow reasonable tolerance for test timing
|
||||
diff := task.Due.Sub(time.Now())
|
||||
expectedDiff := duration
|
||||
tolerance := 5 * time.Second
|
||||
|
||||
if diff < expectedDiff-tolerance || diff > expectedDiff+tolerance {
|
||||
t.Errorf("Expected due date ~%v from now, got %v from now (diff: %v)", duration, diff, diff-expectedDiff)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user