Move terminal colors to global constants #913
Conversation
osterman
added
minor
Cerebrovinny
force-pushed
the
DEV-2799
branch
2 times, most recently
from
e1240cd to
94cc8cc
Compare
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of color and styling management across the Atmos CLI by implementing a centralized Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/utils/version_utils.go (1)
28-35: Consider extracting box style to themeSince this is a reusable UI component, consider moving the box style definition to the theme package for potential reuse across the application.
+ // In pkg/ui/theme/styles.go + var BoxStyle = lipgloss.NewStyle(). + Border(lipgloss.RoundedBorder()). + BorderForeground(lipgloss.Color(ColorGreen)). + Padding(0, 1). + Align(lipgloss.Center) - // In version_utils.go - style := lipgloss.NewStyle(). - Border(lipgloss.RoundedBorder()). - BorderForeground(lipgloss.Color(theme.ColorGreen)). - Padding(0, 1). - Align(lipgloss.Center) + style := theme.BoxStylepkg/ui/theme/colors.go (2)
8-24: Well-organized color constants, consider adding documentation for accessibility.The color constants are well-organized into ANSI and Hex categories. Consider adding documentation about:
- Color contrast ratios for accessibility
- Use cases for each color
- Any brand guidelines these colors follow
// ANSI Colors (for lipgloss) const ( + // Base colors with WCAG 2.1 contrast ratios + // Minimum contrast ratio: 4.5:1 for normal text // Base colors ColorGray = "8" // Version number ColorGreen = "10" // Success, new version
26-51: Strong style organization, consider adding validation for color combinations.The pre-configured styles provide a solid foundation for consistent UI elements. Consider adding:
- Helper methods to validate color combinations
- Documentation for style composition
internal/tui/workflow/column.go (1)
145-145: LGTM! Clean transition to theme-based styling.The change from hardcoded color value to theme constant improves maintainability.
Consider extracting the border style to the theme package as well for complete styling consistency:
-s = s.Border(lipgloss.RoundedBorder()).BorderForeground(lipgloss.Color(theme.ColorBorder)) +s = s.Border(theme.Styles.RoundedBorder).BorderForeground(lipgloss.Color(theme.ColorBorder))pkg/utils/log_utils.go (1)
51-56: Consider extracting error logging pattern.The error logging pattern is repeated multiple times throughout the file. Consider creating a helper function to reduce duplication.
+func logError(err error, prefix string) { + if prefix != "" { + theme.Colors.Error.Println(prefix) + } + theme.Colors.Error.Printf("%s\n", err) +} func LogError(atmosConfig schema.AtmosConfiguration, err error) { if err != nil { - _, printErr := theme.Colors.Error.Fprintln(color.Error, err.Error()+"\n") + _, printErr := theme.Colors.Error.Fprintln(color.Error, err.Error()) if printErr != nil { - theme.Colors.Error.Println("Error logging the error:") - theme.Colors.Error.Printf("%s\n", printErr) - theme.Colors.Error.Println("Original error:") - theme.Colors.Error.Printf("%s\n", err) + logError(printErr, "Error logging the error:") + logError(err, "Original error:") }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cmd/cmd_utils.go(2 hunks)cmd/list_components.go(2 hunks)cmd/list_stacks.go(2 hunks)cmd/validate_stacks.go(2 hunks)internal/exec/atmos.go(3 hunks)internal/exec/terraform_clean.go(3 hunks)internal/exec/vendor_model.go(3 hunks)internal/exec/vendor_model_component.go(2 hunks)internal/exec/workflow_utils.go(3 hunks)internal/tui/atmos/column.go(2 hunks)internal/tui/atmos/list_item.go(1 hunks)internal/tui/templates/templater.go(2 hunks)internal/tui/workflow/column.go(2 hunks)internal/tui/workflow/list_item.go(1 hunks)pkg/config/config.go(2 hunks)pkg/ui/theme/colors.go(1 hunks)pkg/utils/log_utils.go(7 hunks)pkg/utils/version_utils.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- internal/exec/workflow_utils.go
- cmd/cmd_utils.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (15)
internal/tui/atmos/list_item.go (1)
11-11: Clean theme integration!The changes effectively centralize the styling by moving from local style definition to the global theme. This improves consistency and maintainability.
Also applies to: 16-16
internal/tui/workflow/list_item.go (1)
11-11: Excellent consistency with atmos/list_item.go!The parallel changes in both list item implementations demonstrate good adherence to DRY principles and maintain a unified styling approach.
Also applies to: 16-16
cmd/validate_stacks.go (1)
8-8: Clean transition to theme-based success message!The change effectively moves from direct color usage to the centralized theme system while maintaining the same user experience.
Also applies to: 28-28
pkg/utils/version_utils.go (2)
17-18: Clean version number styling!Good use of separate theme styles for current and new versions, improving visual distinction.
20-21: Well-structured link formatting!The links are consistently styled using theme components, making them visually distinct and maintainable.
pkg/ui/theme/colors.go (1)
53-66: LGTM! Clean mapping to legacy color system.The color mapping provides a clean transition path from the old system to the new theme-based approach.
internal/tui/atmos/column.go (1)
76-76: LGTM! Clean theme integration.The border color is now consistently managed through the theme system.
cmd/list_components.go (1)
31-47: LGTM! Consistent error handling with themed output.The implementation correctly uses theme colors for different message types:
- Error messages use theme.Colors.Error
- Warnings use theme.Colors.Warning
- Success messages use theme.Colors.Success
cmd/list_stacks.go (1)
32-47: LGTM! Consistent theme usage across commands.The implementation maintains consistency with other commands in using themed colors for different message types.
pkg/utils/log_utils.go (1)
69-69: Well-structured logging level transitions!The logging functions consistently use theme colors while maintaining clear log level hierarchy.
Also applies to: 78-78, 90-90, 101-101
internal/tui/templates/templater.go (1)
23-30: Excellent style consolidation!The command styles are well-organized and properly utilize the theme package. The style chaining is clean and maintainable.
internal/exec/vendor_model_component.go (1)
15-15: LGTM! Clean transition to centralized theme management.The changes effectively move the spinner style to use the centralized theme package, aligning with the PR's objective.
Also applies to: 40-40
internal/exec/vendor_model.go (1)
71-71: LGTM! Comprehensive theme consolidation.The changes successfully migrate all style definitions to the centralized theme package, maintaining consistency across the codebase.
Also applies to: 75-75, 85-85
pkg/config/config.go (1)
16-16: LGTM! Consistent theme usage for message styling.The changes appropriately transition the message styling to use the centralized theme package.
Also applies to: 209-209
internal/exec/terraform_clean.go (1)
260-260: LGTM! Consistent theme usage for status indicators.The changes successfully migrate all status indicators to use the centralized theme package, maintaining visual consistency across error and success states.
Also applies to: 271-271, 276-276
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cmd/validate_component.go (1)
33-33: Excellent use of semantic color naming! 🎯Using
theme.Colors.Successinstead of direct color instantiation makes the code more maintainable and the intent clearer. The semantic naming clearly communicates when and why this color should be used.Consider documenting the semantic color usage guidelines in the theme package to help other developers choose the appropriate colors for their use cases.
pkg/merge/merge.go (2)
43-43: Remove redundant newline characterSince you're using
Fprintln, which automatically adds a newline, the+"\n"concatenation is redundant.- _, _ = theme.Colors.Error.Fprintln(color.Error, err.Error()+"\n") + _, _ = theme.Colors.Error.Fprintln(color.Error, err.Error())Also applies to: 43-43
43-43: Consider extracting the error printing logicThe error printing pattern is repeated three times. Consider extracting it into a helper function to improve maintainability and reduce duplication.
func printError(err error) { _, _ = theme.Colors.Error.Fprintln(color.Error, err.Error()) }Then use it like:
- _, _ = theme.Colors.Error.Fprintln(color.Error, err.Error()+"\n") + printError(err)Also applies to: 68-68
pkg/logger/logger.go (1)
108-108: Consider removing redundant newline character.The error message already includes a newline from
Fprintln, so concatenating"\n"creates a double newline:- _, err2 := theme.Colors.Error.Fprintln(color.Error, err.Error()+"\n") + _, err2 := theme.Colors.Error.Fprintln(color.Error, err.Error())
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/validate_component.go(2 hunks)internal/exec/atmos.go(3 hunks)pkg/logger/logger.go(4 hunks)pkg/merge/merge.go(3 hunks)pkg/ui/theme/colors.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/exec/atmos.go
- pkg/ui/theme/colors.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
cmd/validate_component.go (1)
10-10: Strong move using the theme package! 💪The transition to a centralized theme package aligns perfectly with the goal of standardizing color usage. This will make future theme changes much easier to manage.
pkg/merge/merge.go (1)
Line range hint
6-6: Verify if the color package is still neededI notice we're still importing the
colorpackage while migrating to the new theme system. The only usage appears to becolor.Erroras the output writer.Let's check if we can fully migrate away from the color package:
Also applies to: 10-10
✅ Verification successful
The color package import is required
The
colorpackage is correctly imported as it provides the essentialcolor.Errorwriter interface used bytheme.Colors.Error.Fprintln(). This is part of the intended design where theme handles styling while color handles the output mechanism.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if color.Error can be replaced with a theme-provided writer ast-grep --pattern 'color.Error'Length of output: 32
Script:
#!/bin/bash # First check the specific file content cat pkg/merge/merge.go # Also search with ripgrep for color.Error usage rg "color\.Error" -t go # And check for any color package imports rg "\"github.com/fatih/color\"" -t goLength of output: 4249
pkg/logger/logger.go (3)
10-10: LGTM! Theme import aligns with standardization goals.The addition of the theme package import supports the move to centralized color management while maintaining compatibility with the existing color package for error handling.
137-137: LGTM! Appropriate color choices for Info and Warning levels.The use of Default and Warning colors respectively for Info and Warning levels follows common logging conventions and provides good visual distinction.
Also applies to: 147-147
120-120: Consider distinct colors for Trace and Debug levels.Both Trace and Debug levels currently use
theme.Colors.Info. Consider using different colors to help distinguish between these log levels visually.Let's check if the theme package provides additional colors we could use:
Also applies to: 128-128
|
💥 This pull request now has conflicts. Could you fix it @Cerebrovinny? 🙏 |
This reverts commit 7c63ea3.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/utils/log_utils.go (3)
51-56: Consider extracting error logging pattern, brave coder!While the theme colors are properly implemented, I notice repeated error printing patterns. Consider extracting this into a helper function to maintain our battle-ready code!
+func logErrorMessage(format string, args ...interface{}) { + theme.Colors.Error.Printf(format+"\n", args...) +} + func LogError(atmosConfig schema.AtmosConfiguration, err error) { if err != nil { _, printErr := theme.Colors.Error.Fprintln(color.Error, err.Error()+"\n") if printErr != nil { - theme.Colors.Error.Println("Error logging the error:") - theme.Colors.Error.Printf("%s\n", printErr) - theme.Colors.Error.Println("Original error:") - theme.Colors.Error.Printf("%s\n", err) + logErrorMessage("Error logging the error:") + logErrorMessage("%s", printErr) + logErrorMessage("Original error:") + logErrorMessage("%s", err) }
69-69: Document our color choices, commander!The theme colors are consistently applied across different log levels. However, let's document why we chose these specific colors to help future warriors understand our strategy!
// LogTrace logs the provided trace message +// Uses theme.Colors.Info to maintain visual hierarchy in detailed logs func LogTrace(atmosConfig schema.AtmosConfiguration, message string) { // LogDebug logs the provided debug message +// Uses theme.Colors.Info to maintain consistency with trace-level messages func LogDebug(atmosConfig schema.AtmosConfiguration, message string) { // LogInfo logs the provided info message +// Uses theme.Colors.Default for standard informational output func LogInfo(atmosConfig schema.AtmosConfiguration, message string) { // LogWarning logs the provided warning message +// Uses theme.Colors.Warning to highlight potential issues func LogWarning(atmosConfig schema.AtmosConfiguration, message string) {Also applies to: 78-78, 90-90, 101-101
110-110: Unify error handling with our helper, strategist!The error handling is consistent with our theme colors, but we can further strengthen our code by using the same helper function we proposed earlier.
if err != nil { - theme.Colors.Error.Printf("%s\n", err) + logErrorMessage("%s", err) }This pattern can be applied to all similar error handling blocks in the function.
Also applies to: 115-115, 120-120, 127-127, 133-133, 139-139
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
cmd/cmd_utils.go(2 hunks)cmd/list_components.go(2 hunks)cmd/list_stacks.go(2 hunks)cmd/validate_component.go(2 hunks)cmd/validate_stacks.go(2 hunks)internal/exec/atmos.go(3 hunks)internal/exec/terraform_clean.go(3 hunks)internal/exec/vendor_model.go(3 hunks)internal/exec/vendor_model_component.go(2 hunks)internal/exec/workflow_utils.go(3 hunks)internal/tui/atmos/column.go(2 hunks)internal/tui/atmos/list_item.go(1 hunks)internal/tui/templates/templater.go(2 hunks)internal/tui/workflow/column.go(2 hunks)internal/tui/workflow/list_item.go(1 hunks)pkg/config/config.go(2 hunks)pkg/logger/logger.go(4 hunks)pkg/merge/merge.go(3 hunks)pkg/ui/theme/colors.go(1 hunks)pkg/utils/log_utils.go(7 hunks)pkg/utils/version_utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- pkg/merge/merge.go
- pkg/utils/version_utils.go
- internal/tui/atmos/column.go
- internal/exec/terraform_clean.go
- internal/exec/vendor_model_component.go
- internal/tui/workflow/list_item.go
- internal/exec/atmos.go
- cmd/validate_stacks.go
- pkg/config/config.go
- internal/tui/workflow/column.go
- cmd/cmd_utils.go
- internal/exec/workflow_utils.go
- internal/tui/atmos/list_item.go
- cmd/validate_component.go
- internal/tui/templates/templater.go
- internal/exec/vendor_model.go
- cmd/list_components.go
- cmd/list_stacks.go
- pkg/logger/logger.go
- pkg/ui/theme/colors.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/utils/log_utils.go (2)
11-12: Clean import additions, warrior!The theme package import alongside the existing color package is well-structured. Both are necessary as we transition to themed colors while maintaining compatibility with color.Error and color.Color types.
Line range hint
1-141: Victory! The theme migration is complete!The changes successfully centralize all color definitions into the theme package while maintaining the existing logging functionality. The code is now more maintainable and consistent.
Let's verify no direct color package usage remains:
✅ Verification successful
HAIL, WARRIOR! The theme migration in utils package stands complete and victorious! 🛡️
While we discovered direct color usage in the separate
pkg/loggerpackage, this is a different battle for another day. The utils package has successfully moved all its color definitions to the theme package as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining direct color package usage rg --type go 'color\.(?:Red|Green|Yellow|Blue|Magenta|Cyan|White|Black)' -g '!pkg/ui/theme/*'Length of output: 593
Script:
#!/bin/bash # Check if logger package is used by utils package rg --type go "logger\." pkg/utils/ # Check for any imports of the logger package rg --type go '^import.*".*logger"' pkg/utils/ # Look for PR description or related issues fd "PULL_REQUEST_TEMPLATE|ISSUE_TEMPLATE" .github/Length of output: 245
Script:
#!/bin/bash # Check PR template content cat .github/PULL_REQUEST_TEMPLATE.md # Look for any migration-related files or documentation fd -g "*MIGRATION*" . fd -g "*UPGRADE*" .Length of output: 686
|
These changes were released in v1.142.0. |
what
Adding new global constants for theme colors
why
stop using different colors through the application
references
Summary by CodeRabbit
Refactor
New Features
Chores