schedule/config.go (1)

30-36: Update NewRemoveOnlyConfig method to align with struct changes

This method still sets the removeOnly field which may cause compilation errors if the field is indeed meant to be removed from the struct.

Please update this method to align with any changes to the struct definition.

schedule/config.go (1)

40-44: Consider adding input validation

The SetCommand method accepts parameters without validation.

Consider adding validation for:

• Empty working directory
• Empty or invalid command
• Nil arguments slice

 func (s *Config) SetCommand(wd, command string, args []string) {
+	if wd == "" {
+		wd = "."
+	}
+	if command == "" {
+		return
+	}
+	if args == nil {
+		args = []string{}
+	}
 	s.WorkingDirectory = wd
 	s.Command = command
 	s.Arguments = NewCommandArguments(args)
 }

schedule/config_test.go (1)

9-40: Enhance test coverage

The test suite could benefit from additional test cases.

Consider adding tests for:

1. SetCommand method with various inputs
2. Error cases and edge cases
3. NewRemoveOnlyConfig method

Example test for SetCommand:

func TestSetCommand(t *testing.T) {
	tests := []struct {
		name    string
		wd      string
		command string
		args    []string
		want    CommandArguments
	}{
		{
			name:    "empty working directory",
			wd:      "",
			command: "test",
			args:    []string{"arg"},
			want:    NewCommandArguments([]string{"arg"}),
		},
		// Add more test cases
	}
	
	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			s := &Config{}
			s.SetCommand(tt.wd, tt.command, tt.args)
			assert.Equal(t, tt.want, s.Arguments)
		})
	}
}

schedule/spaced_title.go (1)

7-18: Add documentation for the spacedTitle function.

The function lacks documentation explaining its purpose and behaviour. Consider adding a comment block describing:

• Function purpose
• Parameter expectations
• Return value format
• Example usage

+// spacedTitle adds spaces before capital letters in a string, except for the first character
+// or when the capital letter follows a space. For example, "ThisIsATest" becomes "This Is A Test".
+// If the input is empty or contains no capital letters, it is returned unchanged.
 func spacedTitle(title string) string {

schedule/spaced_title_test.go (1)

12-16: Consider additional test cases for comprehensive coverage.

While the current test cases are good, consider adding:

• Mixed case words (e.g., "iPhone" → "i Phone")
• Numbers (e.g., "iOS15Device" → "i OS15 Device")
• Special characters (e.g., "user@Home" → "user@ Home")

 	}{
 		{"NoSpacesHere", "No Spaces Here"},
 		{"Already Spaced", "Already Spaced"},
 		{"", ""},
 		{"lowercase", "lowercase"},
 		{"ALLCAPS", "A L L C A P S"},
+		{"iPhone", "i Phone"},
+		{"iOS15Device", "i OS15 Device"},
+		{"user@Home", "user@ Home"},
 	}

schedule/calendar_interval_test.go (1)

11-55: Well-structured table-driven tests with good coverage.

The test cases are well-organised and cover the basic scenarios. However, consider adding these test cases:

• Invalid intervals with out-of-range values (e.g., minute > 59)
• Intervals with overlapping values
• Intervals with all fields populated vs sparse intervals

 		{
 			name:      "Empty intervals",
 			intervals: []CalendarInterval{},
 			expected:  []string{"*-*-* *:*:00"},
 		},
+		{
+			name: "Invalid interval",
+			intervals: []CalendarInterval{
+				{
+					intervalMinute: 60,  // Invalid: minute > 59
+					intervalHour:   24,  // Invalid: hour > 23
+				},
+			},
+			expected: []string{"*-*-* *:*:00"},
+		},
+		{
+			name: "All fields populated",
+			intervals: []CalendarInterval{
+				{
+					intervalMinute:  15,
+					intervalHour:    9,
+					intervalWeekday: 1,
+					intervalDay:     1,
+					intervalMonth:   1,
+				},
+			},
+			expected: []string{"Mon *-01-01 09:15:00"},
+		},

schedule/calendar_interval.go (2)

7-23: Consider using iota for constants and adding value validation.

The constants and type definition are well-documented. However:

1. Consider using iota for the interval constants to ensure uniqueness.
2. Add value validation to ensure the map values are within valid ranges (e.g., 0-59 for minutes).

 const (
-	intervalMinute  = "Minute"
-	intervalHour    = "Hour"
-	intervalWeekday = "Weekday"
-	intervalDay     = "Day"
-	intervalMonth   = "Month"
+	intervalMinute CalendarIntervalType = iota
+	intervalHour
+	intervalWeekday
+	intervalDay
+	intervalMonth
 )
+
+type CalendarIntervalType int
+
+func (t CalendarIntervalType) String() string {
+	return [...]string{"Minute", "Hour", "Weekday", "Day", "Month"}[t]
+}
+
+func (t CalendarIntervalType) isValid(value int) bool {
+	switch t {
+	case intervalMinute:
+		return value >= 0 && value <= 59
+	case intervalHour:
+		return value >= 0 && value <= 23
+	case intervalWeekday:
+		return value >= 0 && value <= 7
+	case intervalDay:
+		return value >= 1 && value <= 31
+	case intervalMonth:
+		return value >= 1 && value <= 12
+	default:
+		return false
+	}
+}

---

67-73: Consider pre-allocating the slice with a more accurate capacity.

The slice is pre-allocated with the length of schedules, but each schedule could generate multiple intervals.

 func getCalendarIntervalsFromSchedules(schedules []*calendar.Event) []CalendarInterval {
-	entries := make([]CalendarInterval, 0, len(schedules))
+	// Estimate capacity: each schedule might generate multiple intervals
+	estimatedCapacity := len(schedules) * 2 // or another reasonable multiplier
+	entries := make([]CalendarInterval, 0, estimatedCapacity)
 	for _, schedule := range schedules {
 		entries = append(entries, getCalendarIntervalsFromScheduleTree(generateTreeOfSchedules(schedule))...)
 	}
 	return entries
 }

systemd/read.go (2)

47-47: Consider making the priority configurable instead of hard-coded
This hard-coded "background" value may not suit all use-cases or environments.

---

86-89: Handle strconv.Atoi parse errors
Currently, parse errors are silently ignored, defaulting to 0. This may cause unexpected behaviour if the user provides a non-integer value. Consider at least logging or returning an error to highlight invalid values.

schedule/handler_systemd.go (3)

97-99: Avoid silently discarding errors
These lines ignore any existing error from getting the systemd status and return nil. This could hinder troubleshooting if the error is legitimate.

---

223-225: Check return value of runSystemctlReload
The reload result is discarded, which might mask issues if systemd fails to reload. Consider capturing or logging the error.

---

244-244: Consider handling runJournalCtlCommand error
Ignoring the error may hide potential problems that occur when retrieving the logs. Logging it could assist debugging.

shell/split_arguments.go (1)

9-32: Add documentation and input validation.

The function logic is correct, but could benefit from the following improvements:

1. Add documentation to explain the function's behaviour with quoted strings and escape characters.
2. Add input validation for empty or nil strings.
3. Add examples to demonstrate usage.

Apply this diff to add documentation and input validation:

+// SplitArguments splits a command line string into individual arguments.
+// It handles quoted strings and escape characters, with platform-specific behaviour:
+// - On non-Windows platforms, backslashes are treated as escape characters
+// - Quoted strings are preserved as single arguments
+// - Spaces outside quotes are used as argument delimiters
+//
+// Example:
+//
+//	SplitArguments(`echo "Hello World" file\ with\ spaces`)
+//	// Returns: []string{"echo", "Hello World", "file with spaces"}
 func SplitArguments(commandLine string) []string {
+	if len(commandLine) == 0 {
+		return []string{}
+	}
 	args := make([]string, 0)

crond/parse_event_test.go (1)

10-46: Add test cases for invalid inputs and edge cases.

The test function has good coverage of valid cron expressions, but could benefit from additional test cases:

1. Invalid cron expressions
2. Edge cases (empty string, invalid day of week, etc.)
3. Comments explaining complex test cases

Apply this diff to add more test cases:

 	testData := []struct {
 		cron     string
 		expected string
+		wantErr  bool
 	}{
 		{"00 00 * * 1,2,3,4,6,0", "Sun..Thu,Sat *-*-* 00:00:00"},
 		{"23 01,02 * * 1,0", "Sun,Mon *-*-* 01,02:23:00"},
+		// Invalid inputs
+		{"", "", true},
+		{"invalid", "", true},
+		{"99 99 * * *", "", true},
+		// Edge cases
+		{"59 23 31 12 7", "", true},  // Invalid day of week
+		{"59 23 31 13 6", "", true},  // Invalid month
 	}

 	for _, testRun := range testData {
 		t.Run(testRun.cron, func(t *testing.T) {
 			event, err := parseEvent(testRun.cron)
+			if testRun.wantErr {
+				require.Error(t, err)
+				return
+			}
 			require.NoError(t, err)
 			assert.Equal(t, testRun.expected, event.String())
 		})
 	}

schedule/command_arguments.go (1)

57-70: Consider edge cases in ConfigFile method.

While the implementation is generally good, it might be worth handling cases where the --config flag appears multiple times in the arguments.

Consider this enhancement:

 func (ca CommandArguments) ConfigFile() string {
 	if len(ca.args) == 0 {
 		return ""
 	}
+	var lastConfig string
 	for i, arg := range ca.args {
 		if arg == "--config" {
 			if i+1 < len(ca.args) {
-				return ca.args[i+1]
+				lastConfig = ca.args[i+1]
 			}
 		}
 	}
-	return ""
+	return lastConfig
 }

crond/parse_event.go (1)

26-37: Consider using a named slice for better readability.

The slice of event fields could be defined as a named variable for better readability and maintainability.

Apply this diff to improve readability:

-	for index, eventField := range []*calendar.Value{
+	eventFields := []*calendar.Value{
 		event.Minute,
 		event.Hour,
 		event.Day,
 		event.Month,
 		event.WeekDay,
-	} {
+	}
+	for index, eventField := range eventFields {

shell/split_arguments_test.go (2)

11-64: Consider grouping test cases by functionality.

The test cases cover different scenarios (basic arguments, quoted strings, platform-specific paths) but are not clearly grouped. Consider using subtests to group related test cases for better organisation and readability.

Apply this diff to improve test structure:

 func TestSplitArguments(t *testing.T) {
-	testCases := []struct {
+	// Basic argument splitting
+	basicCases := []struct {
 		commandLine  string
 		expectedArgs []string
 		windowsMode  bool
 		unixMode     bool
 	}{
 		{
 			commandLine:  `cmd arg1 arg2`,
 			expectedArgs: []string{"cmd", "arg1", "arg2"},
 		},
+	}
+
+	// Quoted string handling
+	quotedCases := []struct {
+		commandLine  string
+		expectedArgs []string
+		windowsMode  bool
+		unixMode     bool
+	}{
 		{
 			commandLine:  `cmd "arg with spaces" arg3`,
 			expectedArgs: []string{"cmd", "arg with spaces", "arg3"},
 		},
+	}
+
+	// Platform-specific cases
+	platformCases := []struct {
+		commandLine  string
+		expectedArgs []string
+		windowsMode  bool
+		unixMode     bool
+	}{
 		{
 			commandLine:  `args --with folder\file.txt`,
 			expectedArgs: []string{"args", "--with", "folder\\file.txt"},
 			windowsMode:  true,
 		},
 	}

---

73-76: Consider using more descriptive test names.

The current test names use the command line as the test name, which can be hard to read. Consider using descriptive names that explain the test's purpose.

Apply this diff to improve test names:

-		t.Run(testCase.commandLine, func(t *testing.T) {
+		testName := fmt.Sprintf("%s/%s",
+			getPlatformPrefix(testCase.windowsMode, testCase.unixMode),
+			getTestDescription(testCase.commandLine))
+		t.Run(testName, func(t *testing.T) {

Add these helper functions:

func getPlatformPrefix(windowsMode, unixMode bool) string {
	if windowsMode {
		return "Windows"
	}
	if unixMode {
		return "Unix"
	}
	return "Common"
}

func getTestDescription(commandLine string) string {
	if strings.Contains(commandLine, "\"") {
		return "QuotedArguments"
	}
	if strings.Contains(commandLine, "\\") {
		return "EscapedPath"
	}
	return "BasicArguments"
}

schedule/handler_crond_test.go (1)

19-51: Consider adding error test cases.

The test cases only cover successful scenarios. Consider adding test cases for error conditions such as invalid schedules, permission issues, or filesystem errors.

Add test cases for error scenarios:

{
	job: Config{
		ProfileName: "invalid",
		Schedules:   []string{"invalid schedule"},
	},
	schedules:    nil,
	expectError:  true,
	errorMessage: "invalid schedule format",
},
{
	job: Config{
		ProfileName: "no.permission",
		Permission:  "root",
	},
	schedules:    []*calendar.Event{hourly},
	expectError:  true,
	errorMessage: "permission denied",
},

schedule/job.go (1)

11-11: Make the error message more descriptive.

The error message "job can be removed only" could be more descriptive to help users understand why the operation failed.

Apply this diff to improve the error message:

-var ErrJobCanBeRemovedOnly = errors.New("job can be removed only")
+var ErrJobCanBeRemovedOnly = errors.New("this job is marked for removal only and cannot be created or modified")

schedule/schedules.go (2)

44-56: Consider using string builder for performance.

While the current implementation is correct, using strings.Builder for concatenating multiple strings in the loop could be more efficient, especially when dealing with many events.

-func displayParsedSchedules(profile, command string, events []*calendar.Event) {
+func displayParsedSchedules(profile, command string, events []*calendar.Event) {
 	now := time.Now().Round(time.Second)
+	var output strings.Builder
 	for index, event := range events {
 		displayHeader(profile, command, index+1, len(events))
 		next := event.Next(now)
-		term.Printf("  Original form: %s\n", event.Input())
-		term.Printf("Normalized form: %s\n", event.String())
-		term.Printf("    Next elapse: %s\n", next.Format(time.UnixDate))
-		term.Printf("       (in UTC): %s\n", next.UTC().Format(time.UnixDate))
-		term.Printf("       From now: %s left\n", next.Sub(now))
+		fmt.Fprintf(&output, "  Original form: %s\n", event.Input())
+		fmt.Fprintf(&output, "Normalized form: %s\n", event.String())
+		fmt.Fprintf(&output, "    Next elapse: %s\n", next.Format(time.UnixDate))
+		fmt.Fprintf(&output, "       (in UTC): %s\n", next.UTC().Format(time.UnixDate))
+		fmt.Fprintf(&output, "       From now: %s left\n", next.Sub(now))
+		term.Print(output.String())
+		output.Reset()
 	}
 	term.Print(platform.LineSeparator)
 }

---

58-74: Consider error handling enhancement.

The error handling for the systemd-analyze command could be more informative by including the schedule that caused the error.

-			return err
+			return fmt.Errorf("failed to analyze schedule %q: %w", schedule, err)

systemd/drop_ins.go (1)

68-69: Good improvements to logging and resource management!

The changes to use Debug level for file operations and the addition of defer statements for file closing are excellent improvements. However, consider handling errors from file closing operations.

-		defer dst.Close()
+		defer func() {
+			if cerr := dst.Close(); cerr != nil && err == nil {
+				err = fmt.Errorf("error closing destination file: %w", cerr)
+			}
+		}()

-		defer src.Close()
+		defer func() {
+			if cerr := src.Close(); cerr != nil && err == nil {
+				err = fmt.Errorf("error closing source file: %w", cerr)
+			}
+		}()

Also applies to: 81-93

schedule/handler_systemd_test.go (1)

15-85: Well-structured test with good cleanup and assertions!

The test is comprehensive and well-structured. However, consider extracting the test cases to a separate variable for better maintainability.

+var systemdTestCases = []struct {
+	job       Config
+	schedules []*calendar.Event
+}{
+	{
+		job: Config{
+			ProfileName:      "testscheduled",
+			CommandName:      "backup",
+			Command:          "/tmp/bin/resticprofile",
+			Arguments:        NewCommandArguments([]string{"--no-ansi", "--config", "examples/dev.yaml", "--name", "self", "check"}),
+			WorkingDirectory: "/resticprofile",
+			Permission:       schedulePermission,
+			ConfigFile:       "examples/dev.yaml",
+			Schedules:        []string{event.String()},
+		},
+		schedules: []*calendar.Event{event},
+	},
+	// ... other test cases
+}

 func TestReadingSystemdScheduled(t *testing.T) {
 	event := calendar.NewEvent()
 	require.NoError(t, event.Parse("2020-01-01"))

 	schedulePermission := constants.SchedulePermissionUser

-	testCases := []struct {
-		job       Config
-		schedules []*calendar.Event
-	}{
-		// ... test cases
-	}
+	testCases := systemdTestCases

commands_schedule_test.go (2)

40-40: Update test documentation to reflect new schedule format.

The schedule format has been changed from "daily" to "*:00,30", which runs every 30 minutes (at minutes 0 and 30 of every hour). Consider adding a comment to explain this schedule pattern for better maintainability.

  profile-schedule-inline:
    backup:
+     # Runs every 30 minutes (at minutes 0 and 30 of every hour)
      schedule: "*:00,30"

Also applies to: 45-45

---

79-79: Update test case descriptions.

The test case expectations have been updated to match the new schedule format, but the test case names could be more descriptive of what they're testing.

-			name: "",
+			name: "empty_profile",

Also applies to: 83-83, 87-87

schedule/schedules_test.go (1)

46-46: Update test documentation for new parameter.

The displayParsedSchedules function now accepts a profile parameter, but the test cases don't document why this parameter was added or how it affects the output.

Add comments explaining the purpose of the profile parameter:

+// TestDisplayParseSchedules verifies that schedules are correctly displayed
+// with profile information included in the output
 func TestDisplayParseSchedules(t *testing.T) {

Also applies to: 60-60, 82-82

schedule/command_arguments_test.go (1)

61-84: LGTM: Comprehensive test coverage for config file handling.

Excellent test coverage for the ConfigFile method. The test cases cover:

• Empty arguments
• Missing config value
• Various path formats
• Different argument positions
• Interaction with other flags

Consider adding test cases for:

+		{[]string{"--config="}, ""},
+		{[]string{"--config=config.toml"}, "config.toml"},

crond/entry.go (1)

81-101: LGTM! Consider adding documentation.

The getter methods are well-structured and maintain proper encapsulation. Consider adding documentation comments to describe the purpose and return values of these methods.

Add documentation comments like this:

+// Event returns the calendar event associated with this entry
 func (e Entry) Event() *calendar.Event {
 	return e.event
 }

systemd/read_test.go (2)

43-75: Consider making test data more descriptive.

The test data could be more descriptive by:

1. Using constants with meaningful names for test data
2. Adding comments explaining the test scenarios

Example improvement:

+const (
+    // Test service unit with background priority and environment variables
+    testServiceUnit = `[Unit]
     Description=resticprofile copy for profile self in examples/linux.yaml
     OnFailure=unit-status-mail@%n.service

---

77-143: Add descriptions to table-driven test cases.

The test cases would be clearer with descriptions explaining each scenario.

Example improvement:

 testCases := []struct {
+    name   string
     config Config
 }{
     {
+        name: "Basic system unit with single schedule",
         config: Config{
             CommandLine:      "/bin/resticprofile --config profiles.yaml run-schedule backup@profile1",

calendar/event.go (1)

99-102: Add comment explaining the time adjustment logic.

The logic for handling non-zero seconds is correct but would benefit from a clear explanation.

Add a comment like this:

 next := from.Truncate(time.Minute) // truncate all the seconds
+// If we're already partway through a minute, skip to the next minute
+// to avoid scheduling events in the past
 if from.Second() > 0 {
     next = next.Add(time.Minute) // it's too late for the current minute
 }

schedule_jobs.go (2)

96-125: Consider adding error recovery for job removal.

The function continues after logging errors but doesn't track which jobs failed to be removed. Consider collecting and returning a slice of errors to provide better feedback to the caller.

 func removeScheduledJobs(handler schedule.Handler, configFile, profileName string) error {
+    var errors []error
     err := handler.Init()
     if err != nil {
         return err
     }
     defer handler.Close()

     clog.Debugf("looking up schedules from configuration file %s", configFile)
     configs, err := handler.Scheduled(profileName)
     if err != nil {
         return err
     }
     if len(configs) == 0 {
         clog.Info("no scheduled jobs found")
         return nil
     }
     for _, cfg := range configs {
         if cfg.ConfigFile != configFile {
             clog.Debugf("skipping job %s/%s from configuration file %s", cfg.ProfileName, cfg.CommandName, cfg.ConfigFile)
             continue
         }
         job := schedule.NewJob(handler, &cfg)
         err = job.Remove()
         if err != nil {
-            clog.Error(err)
+            errors = append(errors, fmt.Errorf("failed to remove job %s/%s: %w", cfg.ProfileName, cfg.CommandName, err))
         }
         clog.Infof("scheduled job %s/%s removed", cfg.ProfileName, cfg.CommandName)
     }
-    return nil
+    if len(errors) > 0 {
+        return fmt.Errorf("failed to remove some jobs: %v", errors)
+    }
+    return nil
 }

---

162-194: Consider adding error recovery for status checks.

Similar to removeScheduledJobs, the statusScheduledJobs function could benefit from collecting errors rather than just logging them.

 func statusScheduledJobs(handler schedule.Handler, configFile, profileName string) error {
+    var errors []error
     err := handler.Init()
     if err != nil {
         return err
     }
     defer handler.Close()

     clog.Debugf("looking up schedules from configuration file %s", configFile)
     configs, err := handler.Scheduled(profileName)
     if err != nil {
         return err
     }
     if len(configs) == 0 {
         clog.Info("no scheduled jobs found")
         return nil
     }
     for _, cfg := range configs {
         if cfg.ConfigFile != configFile {
             clog.Debugf("skipping job %s/%s from configuration file %s", cfg.ProfileName, cfg.CommandName, cfg.ConfigFile)
             continue
         }
         job := schedule.NewJob(handler, &cfg)
         err := job.Status()
         if err != nil {
-            clog.Error(err)
+            errors = append(errors, fmt.Errorf("failed to get status for job %s/%s: %w", cfg.ProfileName, cfg.CommandName, err))
         }
     }
     err = handler.DisplayStatus(profileName)
     if err != nil {
-        clog.Error(err)
+        errors = append(errors, fmt.Errorf("failed to display status: %w", err))
     }
+    if len(errors) > 0 {
+        return fmt.Errorf("failed to get status for some jobs: %v", errors)
+    }
     return nil
 }

schedule/handler_darwin_test.go (1)

185-251: Consider adding negative test cases.

The test covers successful scenarios well, but could benefit from additional test cases that verify error handling:

1. Invalid configuration file paths
2. Permission denied scenarios
3. Network-related failures

 func TestReadingLaunchdScheduled(t *testing.T) {
     // ... existing test cases ...
+    errorTestCases := []struct {
+        name            string
+        job             Config
+        schedules       []*calendar.Event
+        expectedError   string
+    }{
+        {
+            name: "invalid_config_file",
+            job: Config{
+                ProfileName: "invalid",
+                ConfigFile: "/nonexistent/path/config.yaml",
+            },
+            expectedError: "no such file or directory",
+        },
+        {
+            name: "permission_denied",
+            job: Config{
+                ProfileName: "noperm",
+                ConfigFile: "/root/config.yaml",
+            },
+            expectedError: "permission denied",
+        },
+    }
+
+    for _, tc := range errorTestCases {
+        t.Run(tc.name, func(t *testing.T) {
+            handler := NewHandler(SchedulerLaunchd{}).(*HandlerLaunchd)
+            handler.fs = afero.NewMemMapFs()
+
+            _, err := handler.createPlistFile(
+                handler.getLaunchdJob(&tc.job, tc.schedules),
+                tc.job.Permission,
+            )
+            assert.ErrorContains(t, err, tc.expectedError)
+        })
+    }
 }

schedule_jobs_test.go (1)

203-270: Consider adding test cases for concurrent job removal.

The TestRemoveScheduledJobs function could benefit from additional test cases that verify behaviour when multiple jobs are being removed concurrently.

 func TestRemoveScheduledJobs(t *testing.T) {
     // ... existing test cases ...
+    t.Run("concurrent_removal", func(t *testing.T) {
+        handler := mocks.NewHandler(t)
+        handler.EXPECT().Init().Return(nil)
+        handler.EXPECT().Close()
+
+        // Create multiple jobs that will be removed concurrently
+        configs := []schedule.Config{}
+        for i := 0; i < 10; i++ {
+            configs = append(configs, schedule.Config{
+                ProfileName: fmt.Sprintf("profile_%d", i),
+                CommandName: "backup",
+                ConfigFile:  "configFile",
+                Permission:  "user",
+            })
+        }
+
+        handler.EXPECT().Scheduled("").Return(configs, nil)
+
+        // Expect removal of all jobs
+        for _, cfg := range configs {
+            handler.EXPECT().RemoveJob(&cfg, "user").Return(nil)
+        }
+
+        err := removeScheduledJobs(handler, "configFile", "")
+        assert.NoError(t, err)
+    })
 }

commands_schedule.go (1)

120-156: Simplify legacy flag handling in statusSchedule.

The nested if conditions make the code harder to follow. Consider restructuring for better readability.

-    if slices.Contains(flags.resticArgs, "--legacy") { // TODO: remove this option in the future
-        // single profile or group
-        if !slices.Contains(args, "--all") {
+    if slices.Contains(flags.resticArgs, "--legacy") {
+        clog.Warning("The --legacy flag is deprecated and will be removed in version 2.0.0")
+        
+        if slices.Contains(args, "--all") {
+            return handleAllProfilesStatus(c, flags, args)
+        }
+        return handleSingleProfileStatus(c, flags)
+    }

Then extract the logic into separate functions:

func handleSingleProfileStatus(c *config.Config, flags commandLineFlags) error {
    schedulerConfig, schedules, _, err := getScheduleJobs(c, flags)
    if err != nil {
        return err
    }
    if len(schedules) == 0 {
        clog.Warningf("profile or group %s has no schedule", flags.name)
        return nil
    }
    return statusScheduleProfileOrGroup(schedulerConfig, schedules, flags)
}

func handleAllProfilesStatus(c *config.Config, flags commandLineFlags, args []string) error {
    for _, profileName := range selectProfilesAndGroups(c, flags, args) {
        profileFlags := flagsForProfile(flags, profileName)
        scheduler, schedules, schedulable, err := getScheduleJobs(c, profileFlags)
        if err != nil {
            return err
        }
        if len(schedules) == 0 {
            continue
        }
        clog.Infof("%s %q:", cases.Title(language.English).String(schedulable.Kind()), profileName)
        if err := statusScheduleProfileOrGroup(scheduler, schedules, profileFlags); err != nil {
            clog.Error(err)
        }
    }
    return nil
}

schedule/handler_darwin.go (2)

41-42: Consider documenting the namePrefix constant.

Adding a comment explaining the purpose and format of the namePrefix would improve code clarity.

+// namePrefix is the prefix used for all launchd job labels managed by resticprofile
 const namePrefix = "local.resticprofile."

---

209-223: Consider implementing job retrieval error handling.

The Scheduled method could benefit from more robust error handling for file system operations.

 func (h *HandlerLaunchd) Scheduled(profileName string) ([]Config, error) {
+    if h.fs == nil {
+        return nil, fmt.Errorf("file system not initialized")
+    }
     jobs := make([]Config, 0)
     if profileName == "" {
         profileName = "*"
     } else {
         profileName = strings.ToLower(profileName)
     }
     // system jobs
     systemJobs := h.getScheduledJob(profileName, constants.SchedulePermissionSystem)
     jobs = append(jobs, systemJobs...)
     // user jobs
     userJobs := h.getScheduledJob(profileName, constants.SchedulePermissionUser)
     jobs = append(jobs, userJobs...)
     return jobs, nil
 }

examples/v2.yaml (1)

42-42: Remove trailing spaces.

There are trailing spaces on lines 42 and 206 that should be removed.

Apply this diff to remove the trailing spaces:

-    
+
-      
+

Also applies to: 206-206

🧰 Tools

🪛 YAMLlint (1.35.1)

[error] 42-42: trailing spaces

(trailing-spaces)

Makefile (2)

303-308: Enhanced Lint Target with Cross-Platform Testing

The updated lint target now depends on $(GOBIN)/golangci-lint and executes linting for Darwin, Linux, and Windows by setting the GOOS variable. This approach helps catch platform-specific issues. It would be beneficial to add a brief comment explaining why linting under multiple OS environments is required.

---

310-317: Robust Fix Target Incorporating Automated Code Fixes

The revised fix target correctly depends on the golangci-lint installation and performs go mod tidy and go fix before running lint fixes across different GOOS settings. This order ensures that code dependencies are well maintained before applying automatic fixes. Verify that running lint fixes under multiple OS environments does not introduce unintended discrepancies.

crond/io.go (1)

69-69: Remember to handle character sets
A TODO mentions charsets; consider implementing it soon or clarifying encoding expectations.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

main_test.go (1)

23-54: Consider enhancing error handling and cleanup.

The error handling and cleanup could be more robust:

1. Use defer for cleanup to ensure it runs even if tests panic
2. Make error handling consistent across all error cases

Consider this improvement:

 func TestMain(m *testing.M) {
 	tempDir, err := os.MkdirTemp("", "resticprofile")
 	if err != nil {
 		fmt.Printf("cannot create temp dir: %v", err)
 		os.Exit(1)
 	}
+	// Ensure cleanup runs even if tests panic
+	defer os.RemoveAll(tempDir)
+
 	mockBinary = filepath.Join(tempDir, "shellmock")
 	if platform.IsWindows() {
 		mockBinary += ".exe"
 	}
 	cmdMock := exec.Command("go", "build", "-buildvcs=false", "-o", mockBinary, "./shell/mock")
 	if output, err := cmdMock.CombinedOutput(); err != nil {
-		fmt.Fprintf(os.Stderr, "Error building shell/mock binary: %s\nCommand output: %s\n", err, string(output))
+		fmt.Printf("Error building shell/mock binary: %s\nCommand output: %s\n", err, string(output))
 		os.Exit(1)
 	}

 	echoBinary = filepath.Join(tempDir, "shellecho")
 	if platform.IsWindows() {
 		echoBinary += ".exe"
 	}
 	cmdEcho := exec.Command("go", "build", "-buildvcs=false", "-o", echoBinary, "./shell/echo")
 	if output, err := cmdEcho.CombinedOutput(); err != nil {
-		fmt.Fprintf(os.Stderr, "Error building shell/echo binary: %s\nCommand output: %s\n", err, string(output))
+		fmt.Printf("Error building shell/echo binary: %s\nCommand output: %s\n", err, string(output))
 		os.Exit(1)
 	}

 	exitCode := m.Run()
-	_ = os.Remove(mockBinary)
-	_ = os.Remove(echoBinary)
-	_ = os.RemoveAll(tempDir)
 	os.Exit(exitCode)
 }

crond/parse_event_test.go (3)

17-18: Remove duplicate test case.

The test case for "00 00 01 * 3" appears twice consecutively.

 		{"00 00 01 * 3", "Wed *-*-01 00:00:00"},
-		{"00 00 01 * 3", "Wed *-*-01 00:00:00"},

---

15-38: Consider adding comments to explain test scenarios.

The test cases would benefit from comments explaining the specific scenarios being tested, particularly for complex cron expressions. This would make it easier for other developers to understand the test coverage and add new test cases.

Example of how to improve the documentation:

 	}{
+		// Test all weekdays except Friday
 		{"00 00 * * 1,2,3,4,6,0", "Sun..Thu,Sat *-*-* 00:00:00"},
+		// Test multiple hours on specific days
 		{"23 01,02 * * 1,0", "Sun,Mon *-*-* 01,02:23:00"},

---

50-78: Enhance invalid test cases coverage.

Whilst the current test cases cover basic invalid inputs, consider adding:

1. Tests for boundary values (e.g., "60 * * * *" for invalid minutes)
2. Tests for invalid month/day combinations
3. Comments explaining why each input is invalid

Additionally, the multiple space test cases (lines 54-63) could be consolidated into a single parameterised test.

Example of how to improve the test cases:

 	}{
+		// Empty string
 		{""},
+		// Various lengths of whitespace (could be parameterised)
 		{" "},
 		{"  "},
-		{"   "},
-		{"    "},
-		{"     "},
-		{"      "},
-		{"       "},
-		{"        "},
-		{"         "},
+		// Invalid minute value (> 59)
 		{"99 00 * * 0,1"},
+		// Invalid syntax: dash without number
 		{"0- 00 * * 0,1"},
+		// Invalid syntax: dash at start
 		{"-0 00 * * 0,1"},
+		// Invalid syntax: comma without number
 		{"0, 00 * * 0,1"},
+		// Invalid syntax: comma at start
 		{",0 00 * * 0,1"},
+		// Completely invalid format
 		{"invalid"},
+		// Invalid month/day combination
+		{"00 00 31 2 *"},
+		// Invalid hour value (> 23)
+		{"00 24 * * *"},
 	}

schedule/job.go (3)

50-60: Consider wrapping errors for better context.

While the error handling is correct, consider wrapping errors to provide more context about where they occurred. This would make debugging easier.

Apply this diff to wrap errors:

-	if err := j.handler.DisplaySchedules(j.config.ProfileName, j.config.CommandName, j.config.Schedules); err != nil {
+	if err := j.handler.DisplaySchedules(j.config.ProfileName, j.config.CommandName, j.config.Schedules); err != nil {
+		return fmt.Errorf("failed to display schedules: %w", err)
-		return err
 	}

-	schedules, err := j.handler.ParseSchedules(j.config.Schedules)
-	if err != nil {
-		return err
+	schedules, err := j.handler.ParseSchedules(j.config.Schedules)
+	if err != nil {
+		return fmt.Errorf("failed to parse schedules: %w", err)
 	}

-	if err = j.handler.CreateJob(j.config, schedules, permission); err != nil {
-		return err
+	if err = j.handler.CreateJob(j.config, schedules, permission); err != nil {
+		return fmt.Errorf("failed to create job: %w", err)
 	}

Don't forget to add the import:

 import (
 	"errors"
+	"fmt"
 )

---

93-95: Consider wrapping errors for better context.

Similar to the Create method, consider wrapping errors to provide more context about where they occurred.

Apply this diff to wrap errors:

-	if err := j.handler.DisplaySchedules(j.config.ProfileName, j.config.CommandName, j.config.Schedules); err != nil {
-		return err
+	if err := j.handler.DisplaySchedules(j.config.ProfileName, j.config.CommandName, j.config.Schedules); err != nil {
+		return fmt.Errorf("failed to display schedules: %w", err)
 	}

---

97-99: Consider wrapping errors for better context.

Similar to previous segments, consider wrapping errors to provide more context about where they occurred.

Apply this diff to wrap errors:

-	if err := j.handler.DisplayJobStatus(j.config); err != nil {
-		return err
+	if err := j.handler.DisplayJobStatus(j.config); err != nil {
+		return fmt.Errorf("failed to display job status: %w", err)
 	}

schedule/spaced_title.go (1)

1-2: Consider documenting the build constraint.

The build constraint indicates this file is only used on Darwin systems. Consider adding a comment explaining why this is Darwin-specific.

schedule/command_arguments.go (1)

57-74: Consider optimising ConfigFile method.

The method could be more efficient by:

1. Using a single pass through the arguments
2. Avoiding repeated string operations

Consider this optimised implementation:

 func (ca CommandArguments) ConfigFile() string {
-	if len(ca.args) == 0 {
-		return ""
-	}
-	var lastConfig string
-	for i, arg := range ca.args {
-		if arg == "--config" {
-			if i+1 < len(ca.args) {
-				lastConfig = ca.args[i+1]
-			}
-		} else if strings.HasPrefix(arg, "--config=") {
-			lastConfig = strings.TrimPrefix(arg, "--config=")
-		}
-	}
-	return lastConfig
+	const configFlag = "--config"
+	const configPrefix = configFlag + "="
+	
+	var lastConfig string
+	for i := len(ca.args) - 1; i >= 0; i-- {
+		arg := ca.args[i]
+		if arg == configFlag && i+1 < len(ca.args) {
+			return ca.args[i+1]
+		}
+		if strings.HasPrefix(arg, configPrefix) {
+			return arg[len(configPrefix):]
+		}
+	}
+	return ""
 }

commands_schedule.go (1)

118-174: Consider consolidating legacy and new code paths.

The statusSchedule function has significant code duplication between legacy and new implementations. Consider refactoring to reduce duplication.

shell/command_test.go (1)

28-34: LGTM! Good improvements to test setup.

The changes improve the test setup by using a temporary directory and proper path construction. This makes the tests more reliable and prevents potential conflicts.

Consider adding a newline to the error message for better readability:

-		fmt.Printf("cannot create temp dir: %v", err)
+		fmt.Printf("cannot create temp dir: %v\n", err)

schedule_jobs.go (3)

13-57: Consider improving error handling and resource management.

Two suggestions for improvement:

1. Move defer handler.Close() after the error check to avoid calling Close on a failed initialization.
2. Add more context to error messages for better debugging.

Apply this diff:

 func scheduleJobs(handler schedule.Handler, configs []*config.Schedule) error {
     wd, err := os.Getwd()
     if err != nil {
-        return err
+        return fmt.Errorf("failed to get working directory: %w", err)
     }
     binary, err := os.Executable()
     if err != nil {
-        return err
+        return fmt.Errorf("failed to get executable path: %w", err)
     }
 
     err = handler.Init()
     if err != nil {
-        return err
+        return fmt.Errorf("failed to initialize handler: %w", err)
     }
     defer handler.Close()

---

59-94: Enhance error handling in removeJobs function.

Consider adding more context to error messages and improving error handling robustness.

Apply this diff:

 func removeJobs(handler schedule.Handler, configs []*config.Schedule) error {
     err := handler.Init()
     if err != nil {
-        return err
+        return fmt.Errorf("failed to initialize handler: %w", err)
     }
     defer handler.Close()

---

132-165: Improve error handling for DisplayStatus.

Consider propagating the DisplayStatus error instead of just logging it.

Apply this diff:

     err = handler.DisplayStatus(profileName)
     if err != nil {
-        clog.Error(err)
+        return fmt.Errorf("failed to display status: %w", err)
     }
-    return nil
+    return err

schedule_jobs_test.go (1)

203-272: Consider adding more edge cases to the test suite.

The table-driven tests are well-structured, but consider adding these scenarios:

1. Multiple jobs in the same config file
2. Error cases for permission denied

systemd/read.go (3)

15-51: Add input validation and improve error handling.

The function could benefit from:

1. Input validation for the unit name format
2. More specific error messages for different failure scenarios
3. Documentation explaining the expected format and behaviour

Consider applying this improvement:

+// Read parses systemd service and timer unit files and returns their configuration.
+// The unit parameter should be in the format "[email protected]".
+// The unitType parameter determines whether to read from system or user unit directories.
 func Read(unit string, unitType UnitType) (*Config, error) {
+    if !strings.HasSuffix(unit, ".service") {
+        return nil, fmt.Errorf("invalid unit name format: %s (must end with .service)", unit)
+    }
     var err error
     unitsDir := systemdSystemDir

---

113-121: Add input validation and error handling.

The function should validate the input format and return an error for invalid filenames.

Consider applying this improvement:

-func parseServiceFileName(filename string) (profileName, commandName string) {
+func parseServiceFileName(filename string) (profileName, commandName string, err error) {
+    if !strings.HasPrefix(filename, "resticprofile-") || !strings.HasSuffix(filename, ".service") {
+        return "", "", fmt.Errorf("invalid filename format: %s", filename)
+    }
     filename = strings.TrimPrefix(filename, "resticprofile-")
     filename = strings.TrimSuffix(filename, ".service")
-    commandName, profileName, _ = strings.Cut(filename, "@")
+    var found bool
+    commandName, profileName, found = strings.Cut(filename, "@")
+    if !found {
+        return "", "", fmt.Errorf("invalid filename format: missing @ separator")
+    }
     profileName = strings.TrimPrefix(profileName, "profile-")
     return
 }

---

123-130: Add documentation explaining priority mapping.

The function would benefit from documentation explaining the mapping between systemd priorities and schedule priorities.

Consider applying this improvement:

+// getPriority maps systemd CPU scheduling policy to schedule priority constants.
+// - "idle" maps to background priority
+// - all other values map to standard priority
 func getPriority(input string) string {

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

shell/command_test.go (1)

371-380:

🧩 Analysis chain

Consider investigating and fixing the Linux test issue.

The comment indicates an unresolved issue with TestInterruptShellCommand on Linux, leading to the test being skipped. This reduces test coverage on Linux systems.

Let's investigate if this is a known issue:

Would you like me to help investigate the Linux signal handling issue and propose a fix?

---

🏁 Script executed:

#!/bin/bash
# Search for related issues or discussions about Linux signal handling
rg -i "linux.*signal|signal.*linux" -g "!{vendor,dist}/*"

Length of output: 59

---

🏁 Script executed:

#!/bin/bash
# Print the context around TestInterruptShellCommand to see comments and further details
rg -C 5 "func TestInterruptShellCommand" shell/command_test.go

Length of output: 331

Investigate and resolve the skipped Linux test in TestInterruptShellCommand.

The test is currently skipped on Linux due to known signal handling issues, which reduces our Linux test coverage. It would be advantageous to re-investigate the signal handling logic on Linux and update the test or underlying code accordingly.

schedule/spaced_title.go (2)

7-10: Consider adding godoc examples.

The documentation is clear and comprehensive. Consider enhancing it with godoc examples to make it even more useful for developers.

Add examples in godoc format:

 // spacedTitle adds spaces before capital letters in a string, except for the first character
 // or when the capital letter follows a space. For example, "ThisIsATest" becomes "This Is A Test".
 // If the input is empty or contains no capital letters, it is returned unchanged.
 // This function is only used by the launchd handler on macOS.
+//
+// Example:
+//
+//	spacedTitle("ResticProfile") // Returns "Restic Profile"
+//	spacedTitle("ABC")          // Returns "A B C"
+//	spacedTitle("already spaced") // Returns "already spaced"

---

11-22: Consider minor improvements to the implementation.

The implementation is efficient and handles edge cases well. Consider these minor improvements:

 func spacedTitle(title string) string {
+	if title == "" {
+		return title
+	}
+	const (
+		upperA = 'A'
+		upperZ = 'Z'
+	)
 	var previous rune
 	sb := strings.Builder{}
 	for _, char := range title {
-		if char >= 'A' && char <= 'Z' && previous != ' ' && sb.Len() > 0 {
+		if char >= upperA && char <= upperZ && previous != ' ' && sb.Len() > 0 {
 			sb.WriteByte(' ')
 		}
 		sb.WriteRune(char)
 		previous = char
 	}
 	return sb.String()
 }

systemd/read.go (2)

68-68: Use a named constant for the initial sections map size.

The magic number 3 for the initial map size could be replaced with a named constant to improve code maintainability.

+const defaultSectionsMapSize = 3 // Unit, Service, and Timer sections
-	sections := make(map[string]map[string][]string, 3)
+	sections := make(map[string]map[string][]string, defaultSectionsMapSize)

---

128-134: Add error handling for malformed service filenames.

The function assumes a specific filename format but doesn't handle malformed inputs. Consider returning an error for invalid formats.

-func parseServiceFileName(filename string) (profileName, commandName string) {
+func parseServiceFileName(filename string) (profileName, commandName string, err error) {
 	filename = strings.TrimPrefix(filename, "resticprofile-")
+	if filename == "" {
+		return "", "", fmt.Errorf("invalid filename format: missing 'resticprofile-' prefix")
+	}
 	filename = strings.TrimSuffix(filename, ".service")
-	commandName, profileName, _ = strings.Cut(filename, "@")
+	var found bool
+	commandName, profileName, found = strings.Cut(filename, "@")
+	if !found {
+		return "", "", fmt.Errorf("invalid filename format: missing '@' separator")
+	}
 	profileName = strings.TrimPrefix(profileName, "profile-")
+	if profileName == "" {
+		return "", "", fmt.Errorf("invalid filename format: missing profile name")
+	}
 	return
 }

shell/command_test.go (1)

27-49: LGTM! Improved test setup with better resource management.

The changes enhance the test environment setup with proper temporary directory handling and cleanup. The use of an anonymous function for managing defer statements is a good practice.

Consider enhancing the error logging by using t.Logf instead of fmt.Printf for the temporary directory message, as it would integrate better with the test output:

-		fmt.Printf("using temporary dir: %q\n", tempDir)
+		t.Logf("using temporary dir: %q", tempDir)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: creativeprojects
PR: creativeprojects/resticprofile#425
File: systemd/read.go:87-91
Timestamp: 2025-02-04T21:32:13.942Z
Learning: In systemd package, error handling for integer conversion from service files is unnecessary since these files are generated programmatically by the software itself, ensuring clean data.

shell/command_test.go (1)

370-396: Consider improving flaky test handling.

While the comment documents the flakiness issue, there are better ways to handle it:

1. Consider using environment variables to adjust timing expectations in CI.
2. Add retries for flaky tests.

Here's a suggested improvement:

-// Flaky test on github linux runner but can't reproduce locally
+const maxTestDuration = 500 * time.Millisecond
+
+func getMaxTestDuration() time.Duration {
+    if d := os.Getenv("TEST_MAX_DURATION"); d != "" {
+        if parsed, err := time.ParseDuration(d); err == nil {
+            return parsed
+        }
+    }
+    return maxTestDuration
+}
+
 func TestInterruptShellCommand(t *testing.T) {
     t.Parallel()
+    maxRetries := 3
+    var lastErr error
+
     if platform.IsWindows() {
         t.Skip("Test not running on this platform")
     }
-    buffer := &bytes.Buffer{}
-
-    sigChan := make(chan os.Signal, 1)
-
-    cmd := NewSignalledCommand(mockBinary, []string{"test", "--sleep", "3000"}, sigChan)
-    cmd.Stdout = buffer
-
-    // Will ask us to stop in 100ms
-    go func() {
-        time.Sleep(100 * time.Millisecond)
-        sigChan <- syscall.SIGINT
-    }()
-    start := time.Now()
-    _, _, err := cmd.Run()
-    require.Error(t, err)
-
-    // check it ran for more than 100ms (but less than 500ms - the build agent can be very slow at times)
-    duration := time.Since(start)
-    assert.GreaterOrEqual(t, duration.Milliseconds(), int64(100))
-    assert.Less(t, duration.Milliseconds(), int64(500))
+
+    for i := 0; i < maxRetries; i++ {
+        buffer := &bytes.Buffer{}
+        sigChan := make(chan os.Signal, 1)
+        cmd := NewSignalledCommand(mockBinary, []string{"test", "--sleep", "3000"}, sigChan)
+        cmd.Stdout = buffer
+
+        go func() {
+            time.Sleep(100 * time.Millisecond)
+            sigChan <- syscall.SIGINT
+        }()
+
+        start := time.Now()
+        _, _, err := cmd.Run()
+        if err == nil {
+            t.Fatal("Expected an error but got none")
+        }
+
+        duration := time.Since(start)
+        if duration.Milliseconds() >= 100 && duration < getMaxTestDuration() {
+            return // Test passed
+        }
+        lastErr = fmt.Errorf("attempt %d: duration %v outside expected range", i+1, duration)
+        time.Sleep(time.Second) // Wait before retry
+    }
+    t.Fatalf("Test failed after %d attempts: %v", maxRetries, lastErr)
 }

shell/command_test.go (1)

27-47: Well-structured improvements to TestMain!

The changes significantly improve resource management and error handling. The use of an anonymous function with proper defer statements ensures cleanup, while the platform-agnostic path handling enhances cross-platform compatibility.

Consider adding a comment explaining why -buildvcs=false flag is used in the go build command to improve maintainability.

commands_schedule_test.go (1)

40-40: Consider adding test cases for edge cases in schedule format.

The test configuration covers basic schedule formats, but consider adding test cases for:

• Invalid schedule formats
• Schedule formats with seconds
• Schedule formats with specific dates

Also applies to: 45-45

commands_schedule.go (4)

18-20: Add version check for legacy flag warning.

Consider adding a version check to ensure the warning is only displayed for versions before 1.0:

+var currentVersion = "0.9.0" // Replace with actual version

 const (
-	legacyFlagWarning = "the --legacy flag is only temporary and will be removed in version 1.0."
+	legacyFlagWarning = "the --legacy flag is deprecated and will be removed in version 1.0. You are currently using version %s."
 )

---

42-53: Improve concurrent error handling in job collection.

The error handling in the job collection loop could be improved to handle concurrent profile processing:

+type profileError struct {
+    profile string
+    err     error
+}

-		if err != nil {
-			return err
-		}
+		if err != nil {
+            return fmt.Errorf("failed to process profile %s: %w", profileName, err)
+		}

---

93-100: Add success logging for job removal.

Consider adding success logging to provide better visibility of the removal process:

 			err = removeJobs(schedule.NewHandler(schedulerConfig), jobs)
 			if err != nil {
 				err = retryElevated(err, flags)
 			}
 			if err != nil {
 				// we keep trying to remove the other jobs
 				clog.Error(err)
+			} else {
+				clog.Infof("Successfully removed jobs for profile: %s", profileName)
 			}

---

155-156: Enhance status output formatting.

Consider improving the status output format for better readability:

-			clog.Infof("%s %q:", cases.Title(language.English).String(schedulable.Kind()), profileName)
+			clog.Infof("\n%s: %q", cases.Title(language.English).String(schedulable.Kind()), profileName)
+			clog.Infof("=" + strings.Repeat("=", len(profileName)+len(schedulable.Kind())+3))

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

schedule_jobs.go (2)

125-131: Consider enhancing error reporting with failure statistics.

When aggregating errors, it would be helpful to include the total number of failures in the error message.

Here's a suggested improvement:

-		if errs != nil {
-			return fmt.Errorf("failed to remove some jobs: %w", errs)
-		}
+		if errs != nil {
+			failureCount := len(strings.Split(errs.Error(), "\n"))
+			return fmt.Errorf("failed to remove %d jobs: %w", failureCount, errs)
+		}

---

214-237: Consider adding input validation for schedule configuration.

The scheduleToConfig function could benefit from validating critical fields like origin.Name and origin.Command before creating the configuration.

Here's a suggested improvement:

 func scheduleToConfig(sched *config.Schedule) *schedule.Config {
+	if sched == nil {
+		return nil
+	}
 	origin := sched.ScheduleOrigin()
+	if origin.Name == "" || origin.Command == "" {
+		return nil
+	}
 	if !sched.HasSchedules() {
 		// there's no schedule defined, so this record is for removal only
 		return schedule.NewRemoveOnlyConfig(origin.Name, origin.Command)
 	}

schedule/handler_crond.go (1)

125-158: Consider extracting error variable.

The method implementation looks good and aligns with the PR objectives. However, consider extracting the error check condition into a variable for better readability.

-	if configsErr != nil && len(entries) == 0 {
+	hasNoEntries := len(entries) == 0
+	if configsErr != nil && hasNoEntries {

crond/crontab.go (3)

34-53: Consider accepting filesystem in constructor

While the integration of afero.Fs is good, consider accepting it as a constructor parameter for better dependency injection:

-func NewCrontab(entries []Entry) (c *Crontab) {
+func NewCrontab(entries []Entry, fs afero.Fs) (c *Crontab) {
+	if fs == nil {
+		fs = afero.NewOsFs()
+	}
 	c = &Crontab{
 		entries: entries,
-		fs:      afero.NewOsFs(),
+		fs:      fs,
 	}

---

345-389: Document array indices in regex matches

The magic numbers in array indices (e.g., matches[7], matches[9]) make the code harder to maintain. Consider adding constants or comments to document what each index represents:

+	// Legacy pattern matches:
+	// [1] = time expression
+	// [3] = user
+	// [4] = workdir
+	// [5] = command line
+	// [6] = config file
+	// [7] = profile name
+	// [9] = command name
 	matches := legacyPattern.FindStringSubmatch(line)

---

334-336: Improve error message clarity

The error message could be more descriptive by including the actual line content:

-			errs = errors.Join(errs, fmt.Errorf("line %d: %w", lineNum+1, err))
+			errs = errors.Join(errs, fmt.Errorf("line %d %q: %w", lineNum+1, line, err))

commands_schedule_test.go (3)

51-57: Consider using a more realistic crontab example.

The hardcoded crontab example contains some unusual minute patterns (99,40) that would be invalid in a real crontab. While this is a test file, using realistic examples helps catch actual edge cases.

Consider using this more realistic example:

-10,40 * * * *	user	cd /workdir && /home/resticprofile --no-ansi --config config.yaml run-schedule backup@profile
-99,40 * * * *	user	cd /workdir && /home/resticprofile --no-ansi --config config.yaml run-schedule backup@profile
+10,40 * * * *	user	cd /workdir && /home/resticprofile --no-ansi --config config.yaml run-schedule backup@profile
+15,45 * * * *	user	cd /workdir && /home/resticprofile --no-ansi --config config.yaml run-schedule backup@profile

---

169-170: Add test coverage for error cases in statusScheduledJobs.

The test verifies the success path but doesn't cover error cases from statusScheduledJobs.

Consider adding test cases for:

• Invalid crontab file permissions
• Malformed crontab content
• Empty crontab file

---

214-228: Enhance test assertion for schedule creation.

The test verifies that the schedule was created by checking the output string, but it doesn't verify the actual crontab entry.

Add assertions to verify the actual crontab content:

 assert.Contains(t, output.String(), "Original form: *-*-* *:20,50:00")
+// Verify the crontab entry
+crontabContent, err := os.ReadFile(crontab)
+require.NoError(t, err)
+assert.Contains(t, string(crontabContent), "20,50 * * * *")

commands_schedule.go (1)

18-20: Consider using semantic versioning for the legacy flag warning.

The warning message mentions "version 1.0" which could be ambiguous.

Use semantic versioning format:

-	legacyFlagWarning = "the --legacy flag is only temporary and will be removed in version 1.0."
+	legacyFlagWarning = "the --legacy flag is only temporary and will be removed in version 1.0.0."

commands_test.go (1)

353-363: Consider testing edge cases in TestCreateScheduleAll.

The test only verifies the happy path with a default profile.

Add test cases for:

• Multiple profiles with schedules
• Profiles with invalid schedules
• Empty profile list

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: creativeprojects
PR: creativeprojects/resticprofile#425
File: commands_schedule_test.go:49-53
Timestamp: 2025-02-05T20:08:12.633Z
Learning: In unit tests, simple hardcoded examples are preferred over parameterized ones when the goal is to test specific functionality (like schedule parsing) rather than configuration variations. Over-parameterization can unnecessarily complicate test cases.

schedule_jobs.go (2)

96-134: Consider extracting common configuration loading logic.

Both removeScheduledJobs and statusScheduledJobs share similar configuration loading and validation logic. Consider extracting this into a helper function to reduce code duplication.

+func loadScheduledConfigs(handler schedule.Handler, configFile, profileName string) ([]schedule.Config, error) {
+    clog.Debugf("looking up schedules from configuration file %s", configFile)
+    configs, err := handler.Scheduled(profileName)
+    hasNoEntries := len(configs) == 0
+    if err != nil {
+        if hasNoEntries {
+            return nil, err
+        }
+        clog.Errorf("some configurations failed to load:\n%v", err)
+    }
+    if hasNoEntries {
+        clog.Info("no scheduled jobs found")
+        return nil, nil
+    }
+    return configs, nil
+}

Also applies to: 171-214

---

216-239: Consider adding documentation for configuration fields.

The scheduleToConfig function handles complex configuration mapping. Consider adding documentation to explain the purpose and requirements of each field, especially system-specific settings like SystemdDropInFiles and AfterNetworkOnline.

commands_schedule_test.go (1)

59-288: Consider enhancing test coverage and assertions.

While the test coverage is good, consider these improvements:

1. Convert TestRemoveScheduleIntegrationUsingCrontab and TestCreateScheduleIntegrationUsingCrontab to table-driven tests for better coverage of edge cases.
2. Add more specific assertions beyond just string contains checks.

Example of table-driven test structure:

-func TestRemoveScheduleIntegrationUsingCrontab(t *testing.T) {
+func TestRemoveScheduleIntegrationUsingCrontab(t *testing.T) {
+    testCases := []struct {
+        name        string
+        crontab     string
+        profileName string
+        wantErr     error
+        wantContent string
+    }{
+        {
+            name:        "remove existing schedule",
+            crontab:     scheduleIntegrationTestsCrontab,
+            profileName: "profile",
+            wantErr:     nil,
+            wantContent: "11,41 * * * *\tdifferent-script.sh",
+        },
+        // Add more test cases
+    }
+
+    for _, tc := range testCases {
+        t.Run(tc.name, func(t *testing.T) {
+            // Test implementation
+        })
+    }

commands_schedule.go (2)

18-20: Enhance deprecation warning message.

The warning message could be more helpful by including guidance on migration.

-const (
-    legacyFlagWarning = "the --legacy flag is only temporary and will be removed in version 1.0.0"
-)
+const (
+    legacyFlagWarning = "the --legacy flag is only temporary and will be removed in version 1.0.0. Please migrate to the new scheduling format using the standard flags"
+)

---

224-243: Standardise error message format.

Error messages in getProfileScheduleJobs and getGroupScheduleJobs use inconsistent formats.

-return nil, nil, fmt.Errorf("profile '%s': %w", profileName, err)
+return nil, nil, fmt.Errorf("profile %q: %w", profileName, err)

-return nil, nil, fmt.Errorf("cannot load profile '%s': %w", profileName, err)
+return nil, nil, fmt.Errorf("cannot load profile %q: %w", profileName, err)

-return nil, nil, fmt.Errorf("group '%s' not found", profileName)
+return nil, nil, fmt.Errorf("group %q: %w", profileName, config.ErrNotFound)

-return nil, nil, fmt.Errorf("cannot load group '%s': %w", profileName, err)
+return nil, nil, fmt.Errorf("cannot load group %q: %w", profileName, err)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: creativeprojects
PR: creativeprojects/resticprofile#425
File: commands_schedule_test.go:49-53
Timestamp: 2025-02-05T20:08:12.633Z
Learning: In unit tests, simple hardcoded examples are preferred over parameterized ones when the goal is to test specific functionality (like schedule parsing) rather than configuration variations. Over-parameterization can unnecessarily complicate test cases.