Add theme to lets settings#353
Merged
Merged
Conversation
Reviewer's GuideIntroduce a configurable Sequence diagram for applying the new theme setting to CLI color schemessequenceDiagram
actor User
participant CLI as Main
participant Settings as LoadFile
participant Theme as theme
participant Fang as fang.Run
User->>CLI: execute lets
CLI->>Settings: LoadFile(path)
Settings->>Settings: applyEnvOverrides(&cfg)
Settings->>Theme: ValidName(cfg.Theme)
Theme-->>Settings: bool
Settings-->>CLI: Settings{Theme, NoColor, UpgradeNotify}
CLI->>Theme: ColorSchemeByName(appSettings.Theme)
Theme-->>CLI: fang.ColorSchemeFunc
CLI->>Fang: Run(rootCmd, WithColorSchemeFunc(...), ...)
Fang-->>User: themed help and error output
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 3 issues, and left some high level feedback:
- The
LoadFileinvalid-theme error message hardcodes the allowed themes; consider deriving the list from a single source (e.g., a slice in thethemepackage) to avoid it drifting when new themes are added. ColorSchemeByNamesilently falls back to the default for unknown names whileLoadFilerejects invalid themes; consider either documenting this discrepancy or havingColorSchemeByNamesurface an error for unexpected names to better catch programmer mistakes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `LoadFile` invalid-theme error message hardcodes the allowed themes; consider deriving the list from a single source (e.g., a slice in the `theme` package) to avoid it drifting when new themes are added.
- `ColorSchemeByName` silently falls back to the default for unknown names while `LoadFile` rejects invalid themes; consider either documenting this discrepancy or having `ColorSchemeByName` surface an error for unexpected names to better catch programmer mistakes.
## Individual Comments
### Comment 1
<location path="internal/settings/settings.go" line_range="80-81" />
<code_context>
applyEnvOverrides(&cfg)
+ if !theme.ValidName(cfg.Theme) {
+ return Settings{}, fmt.Errorf(
+ "invalid theme %q: must be one of %q, %q, %q",
+ cfg.Theme,
</code_context>
<issue_to_address>
**suggestion:** Avoid hard-coding the list of valid themes in the error message to prevent drift from `theme.ValidName`.
Right now the message hard-codes `DefaultName`, `ANSIName`, and `SynthwaveName`, which can fall out of sync as themes change. Consider exposing the valid theme names from `theme` (e.g., via a `Names()` helper or shared slice) and constructing the error message from that so the validation and error stay aligned.
</issue_to_address>
### Comment 2
<location path="internal/theme/theme_test.go" line_range="35" />
<code_context>
+ t.Fatalf("expected synthwave theme title color %v, got %v", charmtone.Grape, got)
+ }
+
+ if got := ColorSchemeByName("unknown")(lipgloss.LightDark(true)).Title; got != charmtone.Ash {
+ t.Fatalf("expected unknown theme to fall back to %v, got %v", charmtone.Ash, got)
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Reduce coupling of the fallback behavior test to specific color values
The last assertion hardcodes `charmtone.Ash` as the expected Title color for unknown themes, tying the test to the current `DefaultColorScheme` implementation. If that default color changes, this test will fail even though the fallback still works. Instead, compare the scheme for an unknown name to the scheme for `DefaultName`, e.g.:
```go
want := ColorSchemeByName(DefaultName)(lipgloss.LightDark(true))
got := ColorSchemeByName("unknown")(lipgloss.LightDark(true))
if got.Title != want.Title { /* ... */ }
```
Possibly compare multiple fields to assert the fallback behavior without hardcoding specific colors.
Suggested implementation:
```golang
func TestColorSchemeByName(t *testing.T) {
lightDark := lipgloss.LightDark(true)
defaultScheme := ColorSchemeByName(DefaultName)(lightDark)
if defaultScheme.Title != charmtone.Ash {
t.Fatalf("expected default theme title color %v, got %v", charmtone.Ash, defaultScheme.Title)
}
unknownScheme := ColorSchemeByName("unknown")(lightDark)
if unknownScheme.Title != defaultScheme.Title {
t.Fatalf("expected unknown theme to fall back to default title color %v, got %v", defaultScheme.Title, unknownScheme.Title)
}
if unknownScheme.ErrorDetails != defaultScheme.ErrorDetails {
t.Fatalf("expected unknown theme to fall back to default error details color %v, got %v", defaultScheme.ErrorDetails, unknownScheme.ErrorDetails)
}
}
```
If the `ColorScheme` struct has other important fields that must match for a correct fallback (e.g. `Body`, `Subtitle`, etc.), you may want to add analogous comparisons for those fields as well to further strengthen the test without hardcoding specific color values.
</issue_to_address>
### Comment 3
<location path="docs/docs/settings.md" line_range="8" />
<code_context>
`lets` settings control the behavior of `lets` itself.
-Use settings for things like colored output or update notifications. Do not use this file for project commands or runtime env. Project behavior still belongs in `lets.yaml`.
+Use settings for things like colored output, theming, or update notifications. Do not use this file for project commands or runtime env. Project behavior still belongs in `lets.yaml`.
## Settings file location
</code_context>
<issue_to_address>
**suggestion (typo):** Consider expanding the abbreviation "runtime env" to "runtime environment" for clarity.
Spelling this out (e.g., "Do not use this file for project commands or runtime environment.") will make the docs clearer to all readers.
```suggestion
Use settings for things like colored output, theming, or update notifications. Do not use this file for project commands or runtime environment. Project behavior still belongs in `lets.yaml`.
```
</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
+80
to
+81
| if !theme.ValidName(cfg.Theme) { | ||
| return Settings{}, fmt.Errorf( |
There was a problem hiding this comment.
suggestion: Avoid hard-coding the list of valid themes in the error message to prevent drift from theme.ValidName.
Right now the message hard-codes DefaultName, ANSIName, and SynthwaveName, which can fall out of sync as themes change. Consider exposing the valid theme names from theme (e.g., via a Names() helper or shared slice) and constructing the error message from that so the validation and error stay aligned.
| t.Fatalf("expected synthwave theme title color %v, got %v", charmtone.Grape, got) | ||
| } | ||
|
|
||
| if got := ColorSchemeByName("unknown")(lipgloss.LightDark(true)).Title; got != charmtone.Ash { |
There was a problem hiding this comment.
suggestion (testing): Reduce coupling of the fallback behavior test to specific color values
The last assertion hardcodes charmtone.Ash as the expected Title color for unknown themes, tying the test to the current DefaultColorScheme implementation. If that default color changes, this test will fail even though the fallback still works. Instead, compare the scheme for an unknown name to the scheme for DefaultName, e.g.:
want := ColorSchemeByName(DefaultName)(lipgloss.LightDark(true))
got := ColorSchemeByName("unknown")(lipgloss.LightDark(true))
if got.Title != want.Title { /* ... */ }Possibly compare multiple fields to assert the fallback behavior without hardcoding specific colors.
Suggested implementation:
func TestColorSchemeByName(t *testing.T) {
lightDark := lipgloss.LightDark(true)
defaultScheme := ColorSchemeByName(DefaultName)(lightDark)
if defaultScheme.Title != charmtone.Ash {
t.Fatalf("expected default theme title color %v, got %v", charmtone.Ash, defaultScheme.Title)
}
unknownScheme := ColorSchemeByName("unknown")(lightDark)
if unknownScheme.Title != defaultScheme.Title {
t.Fatalf("expected unknown theme to fall back to default title color %v, got %v", defaultScheme.Title, unknownScheme.Title)
}
if unknownScheme.ErrorDetails != defaultScheme.ErrorDetails {
t.Fatalf("expected unknown theme to fall back to default error details color %v, got %v", defaultScheme.ErrorDetails, unknownScheme.ErrorDetails)
}
}If the ColorScheme struct has other important fields that must match for a correct fallback (e.g. Body, Subtitle, etc.), you may want to add analogous comparisons for those fields as well to further strengthen the test without hardcoding specific color values.
| `lets` settings control the behavior of `lets` itself. | ||
|
|
||
| Use settings for things like colored output or update notifications. Do not use this file for project commands or runtime env. Project behavior still belongs in `lets.yaml`. | ||
| Use settings for things like colored output, theming, or update notifications. Do not use this file for project commands or runtime env. Project behavior still belongs in `lets.yaml`. |
There was a problem hiding this comment.
suggestion (typo): Consider expanding the abbreviation "runtime env" to "runtime environment" for clarity.
Spelling this out (e.g., "Do not use this file for project commands or runtime environment.") will make the docs clearer to all readers.
Suggested change
| Use settings for things like colored output, theming, or update notifications. Do not use this file for project commands or runtime env. Project behavior still belongs in `lets.yaml`. | |
| Use settings for things like colored output, theming, or update notifications. Do not use this file for project commands or runtime environment. Project behavior still belongs in `lets.yaml`. |
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 by Sourcery
Introduce a configurable UI theme setting for lets and apply it to help and error output.
New Features:
themesetting withdefault,ansi, andsynthwaveoptions for lets output styling.Enhancements:
Documentation:
themesetting, supported values, defaults, and update the changelog and settings examples accordingly.Tests: