-
Notifications
You must be signed in to change notification settings - Fork 0
feat(config): add YAML config support with JSON schema generation #174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2b7c909
addddfc
9b218ca
9a4d888
129c9a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -45,8 +45,8 @@ func main() { | |||||
| slog.SetDefault(l) | ||||||
|
|
||||||
| ctx := context.Background() | ||||||
| configFile := os.ExpandEnv(fmt.Sprintf("%s/%s/%s", "$HOME", model.COMMAND_BASE_STORAGE_FOLDER, "config.toml")) | ||||||
| daemonConfigService := model.NewConfigService(configFile) | ||||||
| configDir := os.ExpandEnv(fmt.Sprintf("%s/%s", "$HOME", model.COMMAND_BASE_STORAGE_FOLDER)) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The path construction is not platform-independent. It uses a hardcoded path separator
Suggested change
References
|
||||||
| daemonConfigService := model.NewConfigService(configDir) | ||||||
| cfg, err := daemonConfigService.ReadConfigFile(ctx) | ||||||
| if err != nil { | ||||||
| slog.Error("Failed to get daemon config", slog.Any("err", err)) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| package commands | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "os" | ||
|
|
||
| "github.com/invopop/jsonschema" | ||
| "github.com/malamtime/cli/model" | ||
| "github.com/urfave/cli/v2" | ||
| ) | ||
|
|
||
| var SchemaCommand = &cli.Command{ | ||
| Name: "schema", | ||
| Usage: "Generate JSON schema for config file (for IDE autocompletion)", | ||
| Flags: []cli.Flag{ | ||
| &cli.StringFlag{ | ||
| Name: "output", | ||
| Aliases: []string{"o"}, | ||
| Usage: "Output file path (default: stdout)", | ||
| }, | ||
| }, | ||
| Action: func(c *cli.Context) error { | ||
| reflector := &jsonschema.Reflector{ | ||
| AllowAdditionalProperties: false, | ||
| } | ||
|
|
||
| schema := reflector.Reflect(&model.ShellTimeConfig{}) | ||
| schema.Title = "ShellTime Configuration" | ||
| schema.Description = "Configuration schema for shelltime CLI. Supports both YAML (.yaml, .yml) and TOML (.toml) formats." | ||
|
|
||
| schemaJSON, err := json.MarshalIndent(schema, "", " ") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to generate schema: %w", err) | ||
| } | ||
|
|
||
| outputPath := c.String("output") | ||
| if outputPath != "" { | ||
| if err := os.WriteFile(outputPath, schemaJSON, 0644); err != nil { | ||
| return fmt.Errorf("failed to write schema file: %w", err) | ||
| } | ||
| fmt.Printf("Schema written to %s\n", outputPath) | ||
| } else { | ||
| fmt.Println(string(schemaJSON)) | ||
| } | ||
|
|
||
| return nil | ||
| }, | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,8 +9,76 @@ import ( | |||||||||
| "sync" | ||||||||||
|
|
||||||||||
| "github.com/pelletier/go-toml/v2" | ||||||||||
| "gopkg.in/yaml.v3" | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| // configFormat represents the format of a config file | ||||||||||
| type configFormat string | ||||||||||
|
|
||||||||||
| const ( | ||||||||||
| formatTOML configFormat = "toml" | ||||||||||
| formatYAML configFormat = "yaml" | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| // detectFormat returns the format based on file extension | ||||||||||
| func detectFormat(filename string) configFormat { | ||||||||||
| ext := strings.ToLower(filepath.Ext(filename)) | ||||||||||
| switch ext { | ||||||||||
| case ".yaml", ".yml": | ||||||||||
| return formatYAML | ||||||||||
| default: | ||||||||||
| return formatTOML | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // unmarshalConfig unmarshals config data based on format | ||||||||||
| func unmarshalConfig(data []byte, format configFormat, config *ShellTimeConfig) error { | ||||||||||
| switch format { | ||||||||||
| case formatYAML: | ||||||||||
| return yaml.Unmarshal(data, config) | ||||||||||
| default: | ||||||||||
| return toml.Unmarshal(data, config) | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // configFiles represents discovered config files | ||||||||||
| type configFiles struct { | ||||||||||
| baseFile string // config.yaml, config.yml, or config.toml | ||||||||||
| localFile string // config.local.yaml, config.local.yml, or config.local.toml | ||||||||||
| baseFormat configFormat | ||||||||||
| localFormat configFormat | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // findConfigFiles discovers config files in priority order | ||||||||||
| // Priority: config.local.yaml > config.local.yml > config.yaml > config.yml > config.local.toml > config.toml | ||||||||||
|
Comment on lines
+52
to
+53
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment describing the priority order is misleading and doesn't accurately reflect what the function does. The function finds the highest-priority base file and local file separately, rather than using a single flat priority list. The comment should be updated to clarify this.
Suggested change
|
||||||||||
| func findConfigFiles(configDir string) configFiles { | ||||||||||
| result := configFiles{} | ||||||||||
|
|
||||||||||
| // Check for base config files (YAML first, then TOML) | ||||||||||
| baseFiles := []string{"config.yaml", "config.yml", "config.toml"} | ||||||||||
| for _, name := range baseFiles { | ||||||||||
| path := filepath.Join(configDir, name) | ||||||||||
| if _, err := os.Stat(path); err == nil { | ||||||||||
| result.baseFile = path | ||||||||||
| result.baseFormat = detectFormat(name) | ||||||||||
| break | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Check for local config files (YAML first, then TOML) | ||||||||||
| localFiles := []string{"config.local.yaml", "config.local.yml", "config.local.toml"} | ||||||||||
| for _, name := range localFiles { | ||||||||||
| path := filepath.Join(configDir, name) | ||||||||||
| if _, err := os.Stat(path); err == nil { | ||||||||||
| result.localFile = path | ||||||||||
| result.localFormat = detectFormat(name) | ||||||||||
| break | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
Comment on lines
+57
to
+77
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||
|
|
||||||||||
| return result | ||||||||||
| } | ||||||||||
|
|
||||||||||
| var UserShellTimeConfig ShellTimeConfig | ||||||||||
|
|
||||||||||
| type ConfigService interface { | ||||||||||
|
|
@@ -33,14 +101,17 @@ func WithSkipCache() ReadConfigOption { | |||||||||
| } | ||||||||||
|
|
||||||||||
| type configService struct { | ||||||||||
| configFilePath string | ||||||||||
| cachedConfig *ShellTimeConfig | ||||||||||
| mu sync.RWMutex | ||||||||||
| configDir string | ||||||||||
| cachedConfig *ShellTimeConfig | ||||||||||
| mu sync.RWMutex | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func NewConfigService(configFilePath string) ConfigService { | ||||||||||
| // NewConfigService creates a new ConfigService that reads config files from the given directory. | ||||||||||
| // It supports both YAML (.yaml, .yml) and TOML (.toml) formats with priority: | ||||||||||
| // config.local.yaml > config.local.yml > config.yaml > config.yml > config.local.toml > config.toml | ||||||||||
|
Comment on lines
+110
to
+111
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This priority list in the comment is misleading. The service supports a base configuration file and a local override file, which can have different formats. The priority is applied separately for base files and for local files. It would be clearer to document these priorities separately.
Suggested change
|
||||||||||
| func NewConfigService(configDir string) ConfigService { | ||||||||||
| return &configService{ | ||||||||||
| configFilePath: configFilePath, | ||||||||||
| configDir: configDir, | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -121,32 +192,32 @@ func (cs *configService) ReadConfigFile(ctx context.Context, opts ...ReadConfigO | |||||||||
| cs.mu.RUnlock() | ||||||||||
| } | ||||||||||
|
|
||||||||||
| configFile := cs.configFilePath | ||||||||||
| existingConfig, err := os.ReadFile(configFile) | ||||||||||
| // Discover config files with priority | ||||||||||
| files := findConfigFiles(cs.configDir) | ||||||||||
|
|
||||||||||
| // Read base config file | ||||||||||
| if files.baseFile == "" { | ||||||||||
| err = fmt.Errorf("no config file found in %s", cs.configDir) | ||||||||||
| return | ||||||||||
| } | ||||||||||
|
|
||||||||||
| existingConfig, err := os.ReadFile(files.baseFile) | ||||||||||
| if err != nil { | ||||||||||
| err = fmt.Errorf("failed to read config file: %w", err) | ||||||||||
| return | ||||||||||
| } | ||||||||||
|
|
||||||||||
| err = toml.Unmarshal(existingConfig, &config) | ||||||||||
| err = unmarshalConfig(existingConfig, files.baseFormat, &config) | ||||||||||
| if err != nil { | ||||||||||
| err = fmt.Errorf("failed to parse config file: %w", err) | ||||||||||
| return | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Check for local config file and merge if exists | ||||||||||
| // Extract the file extension and construct local config filename | ||||||||||
| ext := filepath.Ext(configFile) | ||||||||||
| if ext != "" { | ||||||||||
| // Get the base name without extension | ||||||||||
| baseName := strings.TrimSuffix(configFile, ext) | ||||||||||
| // Construct local config filename: baseName + ".local" + ext | ||||||||||
| localConfigFile := baseName + ".local" + ext | ||||||||||
| if localConfig, localErr := os.ReadFile(localConfigFile); localErr == nil { | ||||||||||
| // Parse local config and merge with base config | ||||||||||
| // Read and merge local config if exists | ||||||||||
| if files.localFile != "" { | ||||||||||
| if localConfig, localErr := os.ReadFile(files.localFile); localErr == nil { | ||||||||||
| var localSettings ShellTimeConfig | ||||||||||
| if unmarshalErr := toml.Unmarshal(localConfig, &localSettings); unmarshalErr == nil { | ||||||||||
| // Merge local settings into base config | ||||||||||
| if unmarshalErr := unmarshalConfig(localConfig, files.localFormat, &localSettings); unmarshalErr == nil { | ||||||||||
| mergeConfig(&config, &localSettings) | ||||||||||
| } | ||||||||||
|
Comment on lines
+220
to
222
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Errors from unmarshalling the local config file are silently ignored. If a user has a malformed local config, they would likely expect an error message instead of their settings being silently disregarded. It's better to propagate this error. if unmarshalErr := unmarshalConfig(localConfig, files.localFormat, &localSettings); unmarshalErr != nil {
err = fmt.Errorf("failed to parse local config file %s: %w", files.localFile, unmarshalErr)
return
}
mergeConfig(&config, &localSettings) |
||||||||||
| } | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path construction is not platform-independent. It uses a hardcoded path separator
/and relies on shell expansion of$HOME. It's better to useos.UserHomeDir()andfilepath.Joinfor creating paths to ensure cross-platform compatibility. Themodelpackage already provides a helper functionGetBaseStoragePathfor this.References
filepath.Jointo combine segments andos.UserHomeDir()to get the home directory, rather than hardcoding path separators or environment variables like$HOME.