diff --git a/cmd/dot/config.go b/cmd/dot/config.go index d7e48631..39430a9a 100644 --- a/cmd/dot/config.go +++ b/cmd/dot/config.go @@ -6,6 +6,8 @@ import ( "fmt" "os" "path/filepath" + "sort" + "strconv" "strings" "github.com/spf13/cobra" @@ -217,49 +219,76 @@ func runConfigGet(key string) error { // getValidConfigKeys returns all valid configuration keys for completion. func getValidConfigKeys() []string { - return []string{ - "directories.package", - "directories.target", - "directories.manifest", - "logging.level", - "logging.format", - "logging.destination", - "symlinks.mode", - "symlinks.backup_suffix", - "symlinks.backup_dir", - "dotfile.prefix", - "dotfile.translate", - "dotfile.package_name_mapping", - "output.format", - "output.color", - "packages.sort_by", + keys := make([]string, 0, len(configGetters)) + for key := range configGetters { + keys = append(keys, key) } + sort.Strings(keys) + return keys +} + +// configGetters maps every gettable key to its renderer. Kept in lockstep +// with the setters in internal/config: every key config set accepts must +// resolve here, or a freshly set key reads back as unknown. +var configGetters = map[string]func(cfg *dot.ExtendedConfig) string{ + "directories.package": func(c *dot.ExtendedConfig) string { return c.Directories.Package }, + "directories.target": func(c *dot.ExtendedConfig) string { return c.Directories.Target }, + "directories.manifest": func(c *dot.ExtendedConfig) string { return c.Directories.Manifest }, + + "logging.level": func(c *dot.ExtendedConfig) string { return c.Logging.Level }, + "logging.format": func(c *dot.ExtendedConfig) string { return c.Logging.Format }, + "logging.destination": func(c *dot.ExtendedConfig) string { return c.Logging.Destination }, + "logging.file": func(c *dot.ExtendedConfig) string { return c.Logging.File }, + + "symlinks.mode": func(c *dot.ExtendedConfig) string { return c.Symlinks.Mode }, + "symlinks.folding": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Symlinks.Folding) }, + "symlinks.overwrite": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Symlinks.Overwrite) }, + "symlinks.backup": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Symlinks.Backup) }, + "symlinks.backup_suffix": func(c *dot.ExtendedConfig) string { return c.Symlinks.BackupSuffix }, + "symlinks.backup_dir": func(c *dot.ExtendedConfig) string { return c.Symlinks.BackupDir }, + + "ignore.use_defaults": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Ignore.UseDefaults) }, + "ignore.patterns": func(c *dot.ExtendedConfig) string { return strings.Join(c.Ignore.Patterns, ",") }, + "ignore.overrides": func(c *dot.ExtendedConfig) string { return strings.Join(c.Ignore.Overrides, ",") }, + + "dotfile.translate": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Dotfile.Translate) }, + "dotfile.prefix": func(c *dot.ExtendedConfig) string { return c.Dotfile.Prefix }, + "dotfile.package_name_mapping": func(c *dot.ExtendedConfig) string { + return strconv.FormatBool(c.Dotfile.PackageNameMapping) + }, + + "output.format": func(c *dot.ExtendedConfig) string { return c.Output.Format }, + "output.color": func(c *dot.ExtendedConfig) string { return c.Output.Color }, + "output.progress": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Output.Progress) }, + "output.verbosity": func(c *dot.ExtendedConfig) string { return strconv.Itoa(c.Output.Verbosity) }, + "output.width": func(c *dot.ExtendedConfig) string { return strconv.Itoa(c.Output.Width) }, + + "operations.dry_run": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Operations.DryRun) }, + "operations.atomic": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Operations.Atomic) }, + "operations.max_parallel": func(c *dot.ExtendedConfig) string { return strconv.Itoa(c.Operations.MaxParallel) }, + + "packages.sort_by": func(c *dot.ExtendedConfig) string { return c.Packages.SortBy }, + "packages.auto_discover": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Packages.AutoDiscover) }, + "packages.validate_names": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Packages.ValidateNames) }, + + "doctor.auto_fix": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Doctor.AutoFix) }, + "doctor.check_manifest": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Doctor.CheckManifest) }, + "doctor.check_broken_links": func(c *dot.ExtendedConfig) string { + return strconv.FormatBool(c.Doctor.CheckBrokenLinks) + }, + "doctor.check_orphaned": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Doctor.CheckOrphaned) }, + "doctor.check_permissions": func(c *dot.ExtendedConfig) string { + return strconv.FormatBool(c.Doctor.CheckPermissions) + }, + + "experimental.parallel": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Experimental.Parallel) }, + "experimental.profiling": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Experimental.Profiling) }, } // getConfigValue retrieves a value from config by key path. func getConfigValue(cfg *dot.ExtendedConfig, key string) (string, error) { - getters := map[string]func() string{ - "directories.package": func() string { return cfg.Directories.Package }, - "directories.target": func() string { return cfg.Directories.Target }, - "directories.manifest": func() string { return cfg.Directories.Manifest }, - "logging.level": func() string { return cfg.Logging.Level }, - "logging.format": func() string { return cfg.Logging.Format }, - "logging.destination": func() string { return cfg.Logging.Destination }, - "symlinks.mode": func() string { return cfg.Symlinks.Mode }, - "symlinks.backup_suffix": func() string { return cfg.Symlinks.BackupSuffix }, - "symlinks.backup_dir": func() string { return cfg.Symlinks.BackupDir }, - "dotfile.prefix": func() string { return cfg.Dotfile.Prefix }, - "dotfile.translate": func() string { return fmt.Sprintf("%t", cfg.Dotfile.Translate) }, - "dotfile.package_name_mapping": func() string { - return fmt.Sprintf("%t", cfg.Dotfile.PackageNameMapping) - }, - "output.format": func() string { return cfg.Output.Format }, - "output.color": func() string { return cfg.Output.Color }, - "packages.sort_by": func() string { return cfg.Packages.SortBy }, - } - - if getter, ok := getters[key]; ok { - return getter(), nil + if getter, ok := configGetters[key]; ok { + return getter(cfg), nil } return "", fmt.Errorf("unknown config key: %s", key) } diff --git a/cmd/dot/config_test.go b/cmd/dot/config_test.go index 7170386b..318a9ead 100644 --- a/cmd/dot/config_test.go +++ b/cmd/dot/config_test.go @@ -726,3 +726,95 @@ func TestRunConfigListCmd(t *testing.T) { err = runConfigListCmd(cmd, []string{}) assert.NoError(t, err) } + +func TestConfigCommand_EveryCompletedKeyRoundTrips(t *testing.T) { + // Shell completion offers getValidConfigKeys() for BOTH get and set, so + // every completed key must survive set -> load -> get (issue #94: the + // getter and setter hand-lists had drifted, so freshly set keys read + // back as unknown). Driving the loop from getValidConfigKeys keeps the + // invariant self-maintaining: a new getter without a matching setter + // (or sample here) fails this test instead of failing users. + samples := map[string]string{ + "directories.package": "/set/pkgs", + "directories.target": "/set/target", + "directories.manifest": "/set/manifest", + "logging.level": "DEBUG", + "logging.format": "json", + "logging.destination": "stdout", + "logging.file": "/set/dot.log", + "symlinks.mode": "absolute", + "symlinks.folding": "false", + "symlinks.overwrite": "true", + "symlinks.backup": "true", + "symlinks.backup_suffix": ".orig", + "symlinks.backup_dir": "/set/backups", + "ignore.use_defaults": "false", + "ignore.patterns": "*.log,cache", + "ignore.overrides": "keep,also", + "dotfile.translate": "false", + "dotfile.prefix": "custom-", + "dotfile.package_name_mapping": "false", + "output.format": "json", + "output.color": "always", + "output.progress": "false", + "output.verbosity": "2", + "output.width": "120", + "operations.dry_run": "true", + "operations.atomic": "false", + "operations.max_parallel": "4", + "packages.sort_by": "links", + "packages.auto_discover": "false", + "packages.validate_names": "false", + "doctor.auto_fix": "true", + "doctor.check_manifest": "false", + "doctor.check_broken_links": "false", + "doctor.check_orphaned": "false", + "doctor.check_permissions": "false", + "experimental.parallel": "true", + "experimental.profiling": "true", + } + + for _, key := range getValidConfigKeys() { + t.Run(key, func(t *testing.T) { + sample, ok := samples[key] + require.True(t, ok, "key %s is offered for completion but has no sample here; add one and make sure config set accepts it", key) + + configPath := filepath.Join(t.TempDir(), "config.yaml") + writer := config.NewWriter(configPath) + require.NoError(t, writer.WriteDefault(config.WriteOptions{Format: "yaml"})) + + require.NoError(t, writer.Update(key, sample), "config set rejects a key it offers for completion") + + loaded, err := config.LoadExtendedFromFile(configPath) + require.NoError(t, err) + + got, err := getConfigValue(loaded, key) + require.NoError(t, err, "config get rejects a key it offers for completion") + assert.Equal(t, sample, got) + }) + } +} + +func TestConfigCommand_GetRendersTypedValues(t *testing.T) { + cfg := config.DefaultExtended() + cfg.Symlinks.Backup = true + cfg.Output.Verbosity = 2 + cfg.Ignore.Patterns = []string{"*.log", "cache"} + + tests := []struct { + key string + expected string + }{ + {"symlinks.backup", "true"}, + {"output.verbosity", "2"}, + {"ignore.patterns", "*.log,cache"}, + } + + for _, tt := range tests { + t.Run(tt.key, func(t *testing.T) { + value, err := getConfigValue(cfg, tt.key) + require.NoError(t, err) + assert.Equal(t, tt.expected, value) + }) + } +} diff --git a/internal/config/writer.go b/internal/config/writer.go index 8c417f91..13df379f 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "strconv" "strings" "github.com/yaklabco/dot/internal/domain" @@ -212,7 +213,7 @@ func setLoggingValue(cfg *LoggingConfig, field string, value interface{}) error func setSymlinksValue(cfg *SymlinksConfig, field string, value interface{}) error { switch field { - case "mode", "backup_suffix": + case "mode", "backup_suffix", "backup_dir": str, ok := value.(string) if !ok { return fmt.Errorf("symlinks.%s: value must be string", field) @@ -223,12 +224,14 @@ func setSymlinksValue(cfg *SymlinksConfig, field string, value interface{}) erro cfg.Mode = str case "backup_suffix": cfg.BackupSuffix = str + case "backup_dir": + cfg.BackupDir = str } case "folding", "overwrite", "backup": - b, ok := value.(bool) - if !ok { - return fmt.Errorf("symlinks.%s: value must be bool", field) + b, err := toBool(value, "symlinks."+field) + if err != nil { + return err } switch field { @@ -250,9 +253,9 @@ func setSymlinksValue(cfg *SymlinksConfig, field string, value interface{}) erro func setIgnoreValue(cfg *IgnoreConfig, field string, value interface{}) error { switch field { case "use_defaults": - b, ok := value.(bool) - if !ok { - return fmt.Errorf("ignore.%s: value must be bool", field) + b, err := toBool(value, "ignore."+field) + if err != nil { + return err } cfg.UseDefaults = b @@ -330,21 +333,16 @@ func setOutputValue(cfg *OutputConfig, field string, value interface{}) error { } case "progress": - b, ok := value.(bool) - if !ok { - return fmt.Errorf("output.%s: value must be bool", field) + b, err := toBool(value, "output."+field) + if err != nil { + return err } cfg.Progress = b case "verbosity", "width": - var i int - switch v := value.(type) { - case int: - i = v - case float64: - i = int(v) - default: - return fmt.Errorf("output.%s: value must be int", field) + i, err := toInt(value, "output."+field) + if err != nil { + return err } switch field { @@ -364,9 +362,9 @@ func setOutputValue(cfg *OutputConfig, field string, value interface{}) error { func setOperationsValue(cfg *OperationsConfig, field string, value interface{}) error { switch field { case "dry_run", "atomic": - b, ok := value.(bool) - if !ok { - return fmt.Errorf("operations.%s: value must be bool", field) + b, err := toBool(value, "operations."+field) + if err != nil { + return err } switch field { @@ -377,14 +375,9 @@ func setOperationsValue(cfg *OperationsConfig, field string, value interface{}) } case "max_parallel": - var i int - switch v := value.(type) { - case int: - i = v - case float64: - i = int(v) - default: - return fmt.Errorf("operations.%s: value must be int", field) + i, err := toInt(value, "operations."+field) + if err != nil { + return err } cfg.MaxParallel = i @@ -405,9 +398,9 @@ func setPackagesValue(cfg *PackagesConfig, field string, value interface{}) erro cfg.SortBy = str case "auto_discover", "validate_names": - b, ok := value.(bool) - if !ok { - return fmt.Errorf("packages.%s: value must be bool", field) + b, err := toBool(value, "packages."+field) + if err != nil { + return err } switch field { @@ -425,44 +418,48 @@ func setPackagesValue(cfg *PackagesConfig, field string, value interface{}) erro } func setDoctorValue(cfg *DoctorConfig, field string, value interface{}) error { - b, ok := value.(bool) - if !ok { - return fmt.Errorf("doctor.%s: value must be bool", field) - } - + var target *bool switch field { case "auto_fix": - cfg.AutoFix = b + target = &cfg.AutoFix case "check_manifest": - cfg.CheckManifest = b + target = &cfg.CheckManifest case "check_broken_links": - cfg.CheckBrokenLinks = b + target = &cfg.CheckBrokenLinks case "check_orphaned": - cfg.CheckOrphaned = b + target = &cfg.CheckOrphaned case "check_permissions": - cfg.CheckPermissions = b + target = &cfg.CheckPermissions default: return fmt.Errorf("unknown field: doctor.%s", field) } + b, err := toBool(value, "doctor."+field) + if err != nil { + return err + } + *target = b + return nil } func setExperimentalValue(cfg *ExperimentalConfig, field string, value interface{}) error { - b, ok := value.(bool) - if !ok { - return fmt.Errorf("experimental.%s: value must be bool", field) - } - + var target *bool switch field { case "parallel": - cfg.Parallel = b + target = &cfg.Parallel case "profiling": - cfg.Profiling = b + target = &cfg.Profiling default: return fmt.Errorf("unknown field: experimental.%s", field) } + b, err := toBool(value, "experimental."+field) + if err != nil { + return err + } + *target = b + return nil } @@ -506,7 +503,7 @@ func toBool(value interface{}, fieldName string) (bool, error) { case bool: return v, nil case string: - switch strings.ToLower(v) { + switch strings.ToLower(strings.TrimSpace(v)) { case "true": return true, nil case "false": @@ -519,6 +516,24 @@ func toBool(value interface{}, fieldName string) (bool, error) { } } +// toInt converts a value to int, accepting int, float64, and decimal strings. +func toInt(value interface{}, fieldName string) (int, error) { + switch v := value.(type) { + case int: + return v, nil + case float64: + return int(v), nil + case string: + i, err := strconv.Atoi(strings.TrimSpace(v)) + if err != nil { + return 0, fmt.Errorf("%s: value must be an integer, got %q", fieldName, v) + } + return i, nil + default: + return 0, fmt.Errorf("%s: value must be int", fieldName) + } +} + // fileExists checks if a file exists. func fileExists(path string) bool { _, err := os.Stat(path) diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 88d38a9d..d9312051 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -249,3 +249,90 @@ func TestWriter_DetectFormat(t *testing.T) { }) } } + +func TestWriter_UpdateCoercesCLIStrings(t *testing.T) { + // dot config set delivers every value as a string; each typed field must + // coerce it rather than type-assert it (issue #94). + tests := []struct { + key string + value string + read func(cfg *config.ExtendedConfig) interface{} + want interface{} + }{ + {"symlinks.folding", "false", func(c *config.ExtendedConfig) interface{} { return c.Symlinks.Folding }, false}, + {"symlinks.overwrite", "true", func(c *config.ExtendedConfig) interface{} { return c.Symlinks.Overwrite }, true}, + {"symlinks.backup", "true", func(c *config.ExtendedConfig) interface{} { return c.Symlinks.Backup }, true}, + {"ignore.use_defaults", "false", func(c *config.ExtendedConfig) interface{} { return c.Ignore.UseDefaults }, false}, + {"dotfile.translate", "false", func(c *config.ExtendedConfig) interface{} { return c.Dotfile.Translate }, false}, + {"dotfile.package_name_mapping", "false", func(c *config.ExtendedConfig) interface{} { return c.Dotfile.PackageNameMapping }, false}, + {"output.progress", "false", func(c *config.ExtendedConfig) interface{} { return c.Output.Progress }, false}, + {"output.verbosity", "2", func(c *config.ExtendedConfig) interface{} { return c.Output.Verbosity }, 2}, + {"output.width", "120", func(c *config.ExtendedConfig) interface{} { return c.Output.Width }, 120}, + {"operations.dry_run", "true", func(c *config.ExtendedConfig) interface{} { return c.Operations.DryRun }, true}, + {"operations.atomic", "false", func(c *config.ExtendedConfig) interface{} { return c.Operations.Atomic }, false}, + {"operations.max_parallel", "4", func(c *config.ExtendedConfig) interface{} { return c.Operations.MaxParallel }, 4}, + {"packages.auto_discover", "false", func(c *config.ExtendedConfig) interface{} { return c.Packages.AutoDiscover }, false}, + {"packages.validate_names", "false", func(c *config.ExtendedConfig) interface{} { return c.Packages.ValidateNames }, false}, + {"doctor.auto_fix", "true", func(c *config.ExtendedConfig) interface{} { return c.Doctor.AutoFix }, true}, + {"doctor.check_manifest", "false", func(c *config.ExtendedConfig) interface{} { return c.Doctor.CheckManifest }, false}, + {"doctor.check_broken_links", "false", func(c *config.ExtendedConfig) interface{} { return c.Doctor.CheckBrokenLinks }, false}, + {"doctor.check_orphaned", "false", func(c *config.ExtendedConfig) interface{} { return c.Doctor.CheckOrphaned }, false}, + {"doctor.check_permissions", "false", func(c *config.ExtendedConfig) interface{} { return c.Doctor.CheckPermissions }, false}, + {"experimental.parallel", "true", func(c *config.ExtendedConfig) interface{} { return c.Experimental.Parallel }, true}, + {"experimental.profiling", "true", func(c *config.ExtendedConfig) interface{} { return c.Experimental.Profiling }, true}, + {"symlinks.backup_dir", "/set/backups", func(c *config.ExtendedConfig) interface{} { return c.Symlinks.BackupDir }, "/set/backups"}, + // A quoting accident must not change the outcome by type. + {"symlinks.folding", " true ", func(c *config.ExtendedConfig) interface{} { return c.Symlinks.Folding }, true}, + {"output.width", " 120 ", func(c *config.ExtendedConfig) interface{} { return c.Output.Width }, 120}, + } + + for _, tt := range tests { + t.Run(tt.key+"="+tt.value, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + writer := config.NewWriter(configPath) + require.NoError(t, writer.WriteDefault(config.WriteOptions{Format: "yaml"})) + + require.NoError(t, writer.Update(tt.key, tt.value)) + + loaded, err := config.LoadExtendedFromFile(configPath) + require.NoError(t, err) + assert.Equal(t, tt.want, tt.read(loaded)) + }) + } +} + +func TestWriter_UpdateRejectsUnparsableStrings(t *testing.T) { + tests := []struct { + key string + value string + }{ + {"symlinks.backup", "banana"}, + {"output.verbosity", "loud"}, + {"operations.max_parallel", "many"}, + } + + for _, tt := range tests { + t.Run(tt.key+"="+tt.value, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + writer := config.NewWriter(configPath) + require.NoError(t, writer.WriteDefault(config.WriteOptions{Format: "yaml"})) + + err := writer.Update(tt.key, tt.value) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.key) + }) + } +} + +func TestWriter_UpdateUnknownFieldNamesTheField(t *testing.T) { + // An unknown field must be reported as unknown, not as a value error. + configPath := filepath.Join(t.TempDir(), "config.yaml") + writer := config.NewWriter(configPath) + require.NoError(t, writer.WriteDefault(config.WriteOptions{Format: "yaml"})) + + for _, key := range []string{"doctor.bogus", "experimental.bogus"} { + err := writer.Update(key, "banana") + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown field: "+key) + } +}