Add lets user settings for color and update notifications#330
Merged
Add lets user settings for color and update notifications#330
Conversation
Reviewer's GuideIntroduce a user settings system loaded from ~/.config/lets/config.yaml to control color output and upgrade notifications, ensure these settings are applied before logging and respected by update checks, add reusable helpers for lets user config paths, and update tests and docs accordingly. Sequence diagram for CLI startup applying settings and loggingsequenceDiagram
actor User
participant CLI_Main as CLI_Main
participant SettingsPkg as settings
participant ColorLib as color
participant Logging as logging
User->>CLI_Main: Main(version, buildDate)
CLI_Main->>SettingsPkg: Load()
alt settings load error
SettingsPkg-->>CLI_Main: error
CLI_Main->>User: print "lets: settings error" to stderr
CLI_Main-->>User: exit code 1
else settings loaded
SettingsPkg-->>CLI_Main: Settings
CLI_Main->>SettingsPkg: Settings.Apply()
SettingsPkg->>ColorLib: set NoColor based on Settings.NoColor
ColorLib-->>SettingsPkg: NoColor set
CLI_Main->>Logging: InitLogging(stdout, stderr)
Logging-->>CLI_Main: logging configured
CLI_Main-->>User: proceed with command execution
end
Sequence diagram for update check honoring settings and env varssequenceDiagram
actor User
participant CLI_Main as CLI_Main
participant SettingsPkg as settings
participant CLI_Update as cli_update
User->>CLI_Main: run lets command
CLI_Main->>SettingsPkg: Load()
SettingsPkg-->>CLI_Main: Settings
CLI_Main->>CLI_Update: maybeStartUpdateCheck(ctx, version, command, Settings)
CLI_Update->>CLI_Update: interactive = isInteractiveStderr()
CLI_Update->>CLI_Update: call shouldCheckForUpdate(command.Name(), interactive, Settings)
alt non interactive or Settings.UpgradeNotify is false or CI env set
CLI_Update-->>CLI_Main: no update check (nil channel)
else
CLI_Update-->>CLI_Main: start background update check
end
Updated class diagram for settings and user config path helpersclassDiagram
class FileSettings {
+bool NoColor
+bool UpgradeNotify
}
class Settings {
+bool NoColor
+bool UpgradeNotify
+Apply()
}
class settings_pkg {
+Default() Settings
+Load() Settings
+LoadFile(path string) Settings
}
class util_pkg {
+LetsUserDir() string
+LetsUserFile(name string) string
}
class UpdateNotifier {
+writeState(state notifierState) error
+letsStatePath() string
}
class cli_pkg {
+Main(version string, buildDate string) int
+maybeStartUpdateCheck(ctx context.Context, version string, command *cobra.Command, appSettings Settings) (<-chan updateCheckResult, context.CancelFunc)
+shouldCheckForUpdate(commandName string, interactive bool, appSettings Settings) bool
}
class color_pkg {
+NoColor bool
}
FileSettings <.. Settings : decoded_into
settings_pkg --> FileSettings : uses
settings_pkg --> Settings : constructs
settings_pkg --> util_pkg : uses
Settings --> color_pkg : Apply() sets NoColor
cli_pkg --> settings_pkg : loads_settings
cli_pkg --> Settings : passes_to_update_check
cli_pkg --> util_pkg : indirect_via_settings
UpdateNotifier --> util_pkg : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The settings loader always uses
~/.config/lets/config.yamlviaLetsUserFile, but the CLI already supportsLETS_CONFIG_DIRfor configuration; consider honoring that same override for settings so config and settings paths stay consistent. - In
internal/util/LetsUserDir, you manually construct~/.config/letsfromUserHomeDir; usingos.UserConfigDirwould be more portable across platforms and better expresses the intent of locating the user config directory.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The settings loader always uses `~/.config/lets/config.yaml` via `LetsUserFile`, but the CLI already supports `LETS_CONFIG_DIR` for configuration; consider honoring that same override for settings so config and settings paths stay consistent.
- In `internal/util/LetsUserDir`, you manually construct `~/.config/lets` from `UserHomeDir`; using `os.UserConfigDir` would be more portable across platforms and better expresses the intent of locating the user config directory.
## Individual Comments
### Comment 1
<location path="internal/settings/settings.go" line_range="54-56" />
<code_context>
+ defer file.Close()
+
+ var fileSettings FileSettings
+ decoder := yaml.NewDecoder(file)
+ decoder.KnownFields(true)
+ if err := decoder.Decode(&fileSettings); err != nil {
+ return Settings{}, fmt.Errorf("failed to decode settings file: %w", err)
+ }
</code_context>
<issue_to_address>
**issue:** Treat an empty settings file (EOF) as "no settings" instead of a hard error.
With `decoder.Decode` + `KnownFields(true)`, an empty `config.yaml` returns `io.EOF`, which we currently wrap and treat as fatal, exiting the CLI. Please special-case `io.EOF` (e.g. via `errors.Is(err, io.EOF)`) and treat it like a missing config (defaults + env overrides) instead of an error.
</issue_to_address>
### Comment 2
<location path="internal/util/lets_user_dir.go" line_range="9-15" />
<code_context>
+ "path/filepath"
+)
+
+func LetsUserDir() (string, error) {
+ homeDir, err := os.UserHomeDir()
+ if err != nil {
+ return "", fmt.Errorf("failed to get home dir: %w", err)
+ }
+
+ return filepath.Join(homeDir, ".config", "lets"), nil
+}
+
</code_context>
<issue_to_address>
**suggestion:** Consider using os.UserConfigDir for better cross-platform config placement.
This helper hardcodes `~/.config/lets`, which is Unix-specific. Since this is now the central helper for user config paths, using `os.UserConfigDir()` and then `filepath.Join(dir, "lets")` would better follow platform conventions (especially on Windows) and avoid surprising config locations.
Suggested implementation:
```golang
import (
"fmt"
"os"
"path/filepath"
)
```
```golang
func LetsUserDir() (string, error) {
configDir, err := os.UserConfigDir()
if err != nil {
return "", fmt.Errorf("failed to get user config dir: %w", err)
}
return filepath.Join(configDir, "lets"), nil
```
</issue_to_address>
### Comment 3
<location path="internal/settings/settings_test.go" line_range="29-30" />
<code_context>
+func TestLoadFile(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for malformed/invalid YAML in the settings file
There’s no test for syntactically invalid YAML (e.g. `no_color: [`). In this case, `decoder.Decode` should fail and be wrapped with `"failed to decode settings file"`. Please add a subtest that writes malformed YAML and asserts that `LoadFile` returns an error and does not fall back to defaults.
```suggestion
func TestLoadFile(t *testing.T) {
t.Run("returns error on malformed YAML", func(t *testing.T) {
unsetEnv(t, "NO_COLOR")
unsetEnv(t, "LETS_CHECK_UPDATE")
dir := t.TempDir()
path := filepath.Join(dir, "settings.yaml")
if err := os.WriteFile(path, []byte("no_color: ["), 0o600); err != nil {
t.Fatalf("failed to write malformed settings file: %v", err)
}
cfg, err := LoadFile(path)
if err == nil {
t.Fatalf("LoadFile() error = nil, want non-nil error for malformed YAML")
}
var zero Config
if cfg != zero {
t.Fatalf("LoadFile() cfg = %#v, want zero-value config on error", cfg)
}
})
t.Run("uses defaults when file is missing", func(t *testing.T) {
```
</issue_to_address>
### Comment 4
<location path="internal/settings/settings_test.go" line_range="110-119" />
<code_context>
+ })
+}
+
+func TestLoad(t *testing.T) {
+ tmpDir := t.TempDir()
+ t.Setenv("HOME", tmpDir)
+ unsetEnv(t, "NO_COLOR")
+ unsetEnv(t, "LETS_CHECK_UPDATE")
+
+ configPath := filepath.Join(tmpDir, ".config", "lets", "config.yaml")
+ err := os.MkdirAll(filepath.Dir(configPath), 0o755)
+ if err != nil {
+ t.Fatalf("failed to create config dir: %v", err)
+ }
+
+ err = os.WriteFile(configPath, []byte("no_color: true\n"), 0o644)
+ if err != nil {
+ t.Fatalf("failed to write settings file: %v", err)
+ }
+
+ cfg, err := Load()
+ if err != nil {
+ t.Fatalf("Load() error = %v", err)
+ }
+
+ if !cfg.NoColor {
+ t.Fatal("expected loaded no_color to be true")
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for Load() when the config file is missing
Currently this only tests the case where `~/.config/lets/config.yaml` exists. Please add a subtest where no config file is created under the temp HOME and `Load()` is expected to succeed and return the default settings (respecting any env overrides). This will exercise the missing-config defaulting behavior end-to-end via `Load`, not just `LoadFile`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+54
to
+56
| decoder := yaml.NewDecoder(file) | ||
| decoder.KnownFields(true) | ||
| if err := decoder.Decode(&fileSettings); err != nil { |
There was a problem hiding this comment.
issue: Treat an empty settings file (EOF) as "no settings" instead of a hard error.
With decoder.Decode + KnownFields(true), an empty config.yaml returns io.EOF, which we currently wrap and treat as fatal, exiting the CLI. Please special-case io.EOF (e.g. via errors.Is(err, io.EOF)) and treat it like a missing config (defaults + env overrides) instead of an error.
Comment on lines
+9
to
+15
| func LetsUserDir() (string, error) { | ||
| homeDir, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get home dir: %w", err) | ||
| } | ||
|
|
||
| return filepath.Join(homeDir, ".config", "lets"), nil |
There was a problem hiding this comment.
suggestion: Consider using os.UserConfigDir for better cross-platform config placement.
This helper hardcodes ~/.config/lets, which is Unix-specific. Since this is now the central helper for user config paths, using os.UserConfigDir() and then filepath.Join(dir, "lets") would better follow platform conventions (especially on Windows) and avoid surprising config locations.
Suggested implementation:
import (
"fmt"
"os"
"path/filepath"
)func LetsUserDir() (string, error) {
configDir, err := os.UserConfigDir()
if err != nil {
return "", fmt.Errorf("failed to get user config dir: %w", err)
}
return filepath.Join(configDir, "lets"), nil
Comment on lines
+29
to
+30
| func TestLoadFile(t *testing.T) { | ||
| t.Run("uses defaults when file is missing", func(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Add a test for malformed/invalid YAML in the settings file
There’s no test for syntactically invalid YAML (e.g. no_color: [). In this case, decoder.Decode should fail and be wrapped with "failed to decode settings file". Please add a subtest that writes malformed YAML and asserts that LoadFile returns an error and does not fall back to defaults.
Suggested change
| func TestLoadFile(t *testing.T) { | |
| t.Run("uses defaults when file is missing", func(t *testing.T) { | |
| func TestLoadFile(t *testing.T) { | |
| t.Run("returns error on malformed YAML", func(t *testing.T) { | |
| unsetEnv(t, "NO_COLOR") | |
| unsetEnv(t, "LETS_CHECK_UPDATE") | |
| dir := t.TempDir() | |
| path := filepath.Join(dir, "settings.yaml") | |
| if err := os.WriteFile(path, []byte("no_color: ["), 0o600); err != nil { | |
| t.Fatalf("failed to write malformed settings file: %v", err) | |
| } | |
| cfg, err := LoadFile(path) | |
| if err == nil { | |
| t.Fatalf("LoadFile() error = nil, want non-nil error for malformed YAML") | |
| } | |
| var zero Config | |
| if cfg != zero { | |
| t.Fatalf("LoadFile() cfg = %#v, want zero-value config on error", cfg) | |
| } | |
| }) | |
| t.Run("uses defaults when file is missing", func(t *testing.T) { |
Comment on lines
+110
to
+119
| func TestLoad(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| t.Setenv("HOME", tmpDir) | ||
| unsetEnv(t, "NO_COLOR") | ||
| unsetEnv(t, "LETS_CHECK_UPDATE") | ||
|
|
||
| configPath := filepath.Join(tmpDir, ".config", "lets", "config.yaml") | ||
| err := os.MkdirAll(filepath.Dir(configPath), 0o755) | ||
| if err != nil { | ||
| t.Fatalf("failed to create config dir: %v", err) |
There was a problem hiding this comment.
suggestion (testing): Add coverage for Load() when the config file is missing
Currently this only tests the case where ~/.config/lets/config.yaml exists. Please add a subtest where no config file is created under the temp HOME and Load() is expected to succeed and return the default settings (respecting any env overrides). This will exercise the missing-config defaulting behavior end-to-end via Load, not just LoadFile.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
internal/settingsto load~/.config/lets/config.yamlwithno_colorandupgrade_notify~/.config/letspath helpers and reuse them for updater stateTesting
go test ./...Summary by Sourcery
Introduce configurable user settings for CLI behavior and centralize lets user config paths.
New Features:
Enhancements:
Documentation:
Tests: