Colorize Atmos Describe Commands when TTY attached #919
Conversation
📝 WalkthroughWalkthroughThis pull request introduces syntax highlighting capabilities to the Atmos CLI configuration. Key changes include the addition of a Changes
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: 3
🧹 Nitpick comments (9)
pkg/schema/schema.go (2)
41-47: Add field documentationConsider adding documentation comments for each field in the Terminal struct to explain their purpose and valid values.
type Terminal struct { + // MaxWidth is the maximum width of the terminal output in characters MaxWidth int `yaml:"max_width" json:"max_width" mapstructure:"max_width"` + // Pager indicates whether to use a pager for long output Pager bool `yaml:"pager" json:"pager" mapstructure:"pager"` + // Timestamps indicates whether to show timestamps in output Timestamps bool `yaml:"timestamps" json:"timestamps" mapstructure:"timestamps"` + // Colors indicates whether to use ANSI colors in output Colors bool `yaml:"colors" json:"colors" mapstructure:"colors"` + // Unicode indicates whether to use Unicode characters in output Unicode bool `yaml:"unicode" json:"unicode" mapstructure:"unicode"` + // SyntaxHighlighting contains configuration for code syntax highlighting SyntaxHighlighting SyntaxHighlighting `yaml:"syntax_highlighting" json:"syntax_highlighting" mapstructure:"syntax_highlighting"` }
49-56: Add validation tags for configurationConsider adding validation tags to ensure configuration values are valid:
type SyntaxHighlighting struct { Enabled bool `yaml:"enabled" json:"enabled" mapstructure:"enabled"` - Lexer string `yaml:"lexer" json:"lexer" mapstructure:"lexer"` + Lexer string `yaml:"lexer" json:"lexer" mapstructure:"lexer" validate:"omitempty,oneof=json yaml"` - Formatter string `yaml:"formatter" json:"formatter" mapstructure:"formatter"` + Formatter string `yaml:"formatter" json:"formatter" mapstructure:"formatter" validate:"required,oneof=terminal html"` - Style string `yaml:"style" json:"style" mapstructure:"style"` + Style string `yaml:"style" json:"style" mapstructure:"style" validate:"required"` Pager bool `yaml:"pager" json:"pager" mapstructure:"pager"` Options HighlightOptions `yaml:"options" json:"options" mapstructure:"options"` }atmos.yaml (1)
321-334: Consider consolidating pager settingsThe configuration has two separate pager settings:
settings.terminal.pager: true(line 322)settings.terminal.syntax_highlighting.pager: true(line 331)This could lead to confusion about which setting takes precedence.
Consider consolidating these into a single pager setting under
settings.terminalto avoid potential conflicts.website/docs/cli/configuration/terminal.mdx (1)
83-90: Documentation looks great! Consider adding more examples.The examples section effectively demonstrates basic usage.
Consider adding examples for:
- Using different color schemes
- Enabling/disabling line numbers
- Using the pager feature
pkg/utils/yaml_utils.go (1)
35-41: Consider simplifying the type switchThe type switch implementation could be more concise.
Consider using this more idiomatic approach:
-var atmosConfig schema.AtmosConfiguration -switch v := data.(type) { -case schema.AtmosConfiguration: - atmosConfig = v -case *schema.AtmosConfiguration: - atmosConfig = *v -} +atmosConfig, ok := data.(schema.AtmosConfiguration) +if !ok { + if ptr, ok := data.(*schema.AtmosConfiguration); ok { + atmosConfig = *ptr + } +}pkg/utils/highlight_utils.go (3)
19-31: LGTM! Consider documenting available styles.The default settings are well-chosen, with Dracula theme and line numbers enabled by default.
Consider adding a code comment listing the available styles from Chroma (e.g., "dracula", "monokai", etc.) to help users customize their experience.
33-54: Add validation for formatter and style values.While the function handles defaults well, it should validate that user-provided formatters and styles are actually available in Chroma.
if settings.Style == "" { settings.Style = defaults.Style +} else if styles.Get(settings.Style) == nil { + settings.Style = defaults.Style } if settings.Formatter == "" { settings.Formatter = defaults.Formatter +} else if formatters.Get(settings.Formatter) == nil { + settings.Formatter = defaults.Formatter }
56-67: Enhance error handling specificity.The error handling could be more specific to help users understand and troubleshoot issues.
err := quick.Highlight(&buf, code, lexerName, "terminal", style) if err != nil { - return code, err + return code, fmt.Errorf("failed to highlight code with lexer %s: %w", lexerName, err) }pkg/config/config.go (1)
57-75: Consider dynamic terminal width handling.Terminal width is fetched during config initialization, which won't update if the terminal is resized.
Consider making the terminal width dynamic:
- Move
MaxWidthto a separate function that can be called when needed- Add a window resize signal handler to update the width
Example:
func GetCurrentTerminalWidth() int { width := templates.GetTerminalWidth() if width == 0 { return 80 // sensible default } return width }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
atmos.yaml(1 hunks)internal/tui/templates/templater.go(2 hunks)pkg/config/config.go(2 hunks)pkg/schema/schema.go(1 hunks)pkg/utils/highlight_utils.go(1 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/yaml_utils.go(1 hunks)website/docs/cli/configuration/terminal.mdx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/tui/templates/templater.go (1)
75-76: Well-structured terminal width detection!Good job on exporting the GetTerminalWidth function. The implementation follows best practices with proper terminal detection and fallback handling.
pkg/utils/highlight_utils.go (2)
124-128: LGTM! Clean struct design.The struct is well-designed with clear purpose and follows Go conventions.
130-136: LGTM! Clear constructor implementation.The constructor follows Go conventions and is properly implemented.
Cerebrovinny
force-pushed
the
tty-highlight
branch
from
1cb81ea to
4406a5b
Compare
osterman
added
minor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
pkg/utils/highlight_utils.go (1)
138-145:⚠️ Potential issueFix Write method to handle partial writes correctly.
The Write method still has the issue mentioned in the past review.
func (h *HighlightWriter) Write(p []byte) (n int, err error) { highlighted, err := HighlightCodeWithConfig(string(p), h.config) if err != nil { return 0, err } - return h.writer.Write([]byte(highlighted)) + // Return original length even if highlighting changes the size + n, err = h.writer.Write([]byte(highlighted)) + if err != nil { + return 0, err + } + return len(p), nil }
🧹 Nitpick comments (3)
website/docs/cli/configuration/terminal.mdx (2)
12-20: Enhance the introduction with TTY-specific information.The introduction should mention that syntax highlighting is automatically disabled when TTY is not attached, as this is a key feature mentioned in the PR objectives.
<Intro> -Atmos provides configurable terminal settings that allow you to customize the output appearance, including syntax highlighting for YAML and JSON outputs. These settings can be configured in your `atmos.yaml` configuration file. +Atmos provides configurable terminal settings that allow you to customize the output appearance, including syntax highlighting for YAML and JSON outputs when a TTY is attached. These settings can be configured in your `atmos.yaml` configuration file. When output is redirected to a non-TTY destination (e.g., a file or pipe), syntax highlighting is automatically disabled for compatibility. </Intro>
91-97: Consider adding a visual example.The note about pipe behavior would be more impactful with a visual example showing the difference between TTY and non-TTY output.
atmos.yaml (1)
327-334: Add performance considerations to configuration comments.Consider adding comments about performance implications of certain settings:
- Line numbers can impact rendering performance for large outputs
- Pager affects memory usage for large outputs
syntax_highlighting: enabled: true formatter: terminal # Output formatter (e.g., terminal, html) style: dracula # Highlighting style - pager: true # Enable pager + pager: true # Enable pager (recommended for large outputs to manage memory usage) options: - line_numbers: true # Display line numbers + line_numbers: true # Display line numbers (may impact rendering performance for large outputs) wrap: false # Wrap long lines
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
atmos.yaml(1 hunks)internal/tui/templates/templater.go(2 hunks)pkg/config/config.go(2 hunks)pkg/schema/schema.go(1 hunks)pkg/utils/highlight_utils.go(1 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/yaml_utils.go(1 hunks)website/docs/cli/configuration/terminal.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/templates/templater.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (6)
pkg/schema/schema.go (3)
41-47: 'Terminal' struct extension looks goodThe addition of
SyntaxHighlightingto theTerminalstruct is consistent and well-structured.
49-56: 'SyntaxHighlighting' struct is well-definedFields are appropriately named and typed, providing flexibility for syntax highlighting configurations.
58-60: 'HighlightOptions' struct is clear and conciseThe options for line numbers and wrapping are straightforward.
pkg/utils/yaml_utils.go (1)
34-48: Enhanced YAML printing logic is solidThe updated
PrintAsYAMLfunction appropriately adds syntax highlighting while maintaining fallback to plain text.pkg/utils/json_utils.go (2)
29-34: Consider extracting common configuration logicThe logic for extracting
AtmosConfigurationis duplicated fromyaml_utils.go. Extracting this into a shared utility function would enhance maintainability.
27-41: JSON printing enhancement aligns with YAML logicThe updated
PrintAsJSONfunction adds syntax highlighting similar toPrintAsYAML, ensuring consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/utils/config_utils.go (1)
8-17: Clean implementation, warrior! Consider error handling enhancement.The function handles all cases well with a clean type switch. For better error tracking, consider adding an error return to indicate when the default case is hit.
-func ExtractAtmosConfig(data any) schema.AtmosConfiguration { +func ExtractAtmosConfig(data any) (schema.AtmosConfiguration, error) { switch v := data.(type) { case schema.AtmosConfiguration: - return v + return v, nil case *schema.AtmosConfiguration: - return *v + return *v, nil default: - return schema.AtmosConfiguration{} + return schema.AtmosConfiguration{}, fmt.Errorf("unsupported type: %T", data) } }website/docs/cli/configuration/terminal.mdx (2)
35-98: Consider enhancing the syntax highlighting documentation.While the configuration options are well-documented, users would benefit from visual examples of different styles (vim, monokai, github, dracula) to help them choose the most suitable one.
100-146: Consider expanding the examples section.While the current examples cover basic usage, consider adding:
- Examples of different output formats (YAML vs JSON)
- Examples with different style configurations
- Examples showing line numbers and wrapping options
pkg/schema/schema.go (1)
49-56: Align field names with YAML tags for consistency.The struct uses
UsePageras the field name butpageras the YAML tag. Consider using consistent naming:type SyntaxHighlighting struct { Enabled bool `yaml:"enabled" json:"enabled" mapstructure:"enabled"` Lexer string `yaml:"lexer" json:"lexer" mapstructure:"lexer"` Formatter string `yaml:"formatter" json:"formatter" mapstructure:"formatter"` Style string `yaml:"style" json:"style" mapstructure:"style"` - UsePager bool `yaml:"pager" json:"pager" mapstructure:"pager"` + UsePager bool `yaml:"use_pager" json:"use_pager" mapstructure:"use_pager"` Options HighlightOptions `yaml:"options" json:"options" mapstructure:"options"` }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/docs.go(1 hunks)pkg/config/config.go(2 hunks)pkg/schema/schema.go(1 hunks)pkg/utils/config_utils.go(1 hunks)pkg/utils/highlight_utils.go(1 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/yaml_utils.go(1 hunks)website/docs/cli/configuration/terminal.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/utils/highlight_utils.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (8)
cmd/docs.go (1)
Line range hint
104-108: Well-executed configuration update, champion!The change properly handles the transition to the new configuration structure while maintaining backward compatibility. The deprecation warning helps users migrate to the new setting.
pkg/utils/yaml_utils.go (1)
34-42: Solid implementation with proper fallback, warrior!The code gracefully handles highlighting failures by falling back to plain text, ensuring reliable output in all scenarios.
pkg/utils/json_utils.go (1)
27-34: Strong implementation with proper error handling, champion!The code maintains consistency with the YAML utils implementation while preserving JSON-specific formatting. The fallback mechanism ensures reliable output.
website/docs/cli/configuration/terminal.mdx (2)
1-20: Well-structured introduction and key points!The documentation clearly outlines the purpose and main features of the terminal settings configuration.
21-33: Clear and comprehensive general settings documentation!The YAML example effectively demonstrates all available terminal configuration options.
pkg/config/config.go (1)
57-75:⚠️ Potential issueAlign default settings with documentation.
The default settings in the code don't match those documented in terminal.mdx:
UsePageris true here but documented as falseLineNumbersis true here but documented as falseApply this diff to align with the documented defaults:
SyntaxHighlighting: schema.SyntaxHighlighting{ Enabled: true, Formatter: "terminal", Style: "dracula", - UsePager: true, + UsePager: false, Options: schema.HighlightOptions{ - LineNumbers: true, + LineNumbers: false, Wrap: false, }, },Likely invalid or redundant comment.
pkg/schema/schema.go (2)
41-47: Well-structured Terminal configuration!The Terminal struct effectively encapsulates all terminal-related settings.
58-60: Well-defined highlighting options!The HighlightOptions struct provides a clean interface for configuring line numbers and wrapping.
Cerebrovinny
force-pushed
the
tty-highlight
branch
from
f54263f to
64c2ab1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/schema/schema.go (1)
48-55: Consider renaming the Pager field to avoid ambiguity.The
Pagerfield in bothTerminalandSyntaxHighlightingstructs could cause confusion. Consider renamingHighlightedOutputPagerto be more specific.Apply this diff to clarify the field's purpose:
type SyntaxHighlighting struct { Enabled bool `yaml:"enabled" json:"enabled" mapstructure:"enabled"` Lexer string `yaml:"lexer" json:"lexer" mapstructure:"lexer"` Formatter string `yaml:"formatter" json:"formatter" mapstructure:"formatter"` Theme string `yaml:"theme" json:"theme" mapstructure:"theme"` - HighlightedOutputPager bool `yaml:"pager" json:"pager" mapstructure:"pager"` + UseHighlightedPager bool `yaml:"pager" json:"pager" mapstructure:"pager"` LineNumbers bool `yaml:"line_numbers" json:"line_numbers" mapstructure:"line_numbers"` Wrap bool `yaml:"wrap" json:"wrap" mapstructure:"wrap"` }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
atmos.yaml(1 hunks)cmd/docs.go(1 hunks)internal/tui/templates/templater.go(2 hunks)pkg/config/config.go(2 hunks)pkg/schema/schema.go(1 hunks)pkg/utils/config_utils.go(1 hunks)pkg/utils/highlight_utils.go(1 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/yaml_utils.go(1 hunks)website/docs/cli/configuration/markdown-styling.mdx(0 hunks)website/docs/cli/configuration/terminal.mdx(1 hunks)
💤 Files with no reviewable changes (1)
- website/docs/cli/configuration/markdown-styling.mdx
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/config/config.go
- pkg/utils/config_utils.go
- atmos.yaml
- website/docs/cli/configuration/terminal.mdx
- cmd/docs.go
- internal/tui/templates/templater.go
- pkg/utils/yaml_utils.go
- pkg/utils/highlight_utils.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/utils/json_utils.go (1)
27-34: LGTM! Robust implementation of syntax highlighting.The changes effectively implement syntax highlighting with proper error handling and fallback mechanism. This aligns well with the PR's objective to colorize Atmos Describe commands.
pkg/schema/schema.go (1)
41-46: LGTM! Well-structured Terminal configuration.The Terminal struct maintains a clean organization while incorporating the new syntax highlighting capabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/schema/schema.go (1)
41-46: LGTM! The Terminal struct is well-organized.The field organization and tagging are consistent with Go conventions. The integration of syntax highlighting capabilities enhances the terminal configuration options.
Consider clarifying the distinction between pager settings.
The presence of both
PagerinTerminalandHighlightedOutputPagerinSyntaxHighlightingmight cause confusion. Consider adding documentation to clarify their distinct purposes:
Terminal.Pager: Controls paging for all terminal outputSyntaxHighlighting.HighlightedOutputPager: Specifically for syntax-highlighted content
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/config/config.go(2 hunks)pkg/schema/schema.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/config/config.go (1)
57-73: LGTM! The terminal settings structure is well-designed.The implementation provides a comprehensive set of terminal configuration options, including syntax highlighting capabilities, which aligns well with the PR objective of colorizing Atmos Describe commands.
Standardize default settings across all files.
The default settings here don't match those in
atmos.yamlandhighlight_utils.go:
LineNumbersis true here but false in atmos.yamlPageris true here but false in atmos.yamlSyntaxHighlighting: schema.SyntaxHighlighting{ Enabled: true, Formatter: "terminal", Theme: "dracula", - HighlightedOutputPager: true, + HighlightedOutputPager: false, LineNumbers: true, - LineNumbers: true, + LineNumbers: false, Wrap: false, },pkg/schema/schema.go (1)
48-55: LGTM! The SyntaxHighlighting struct provides comprehensive configuration options.The implementation offers a robust set of options for customizing syntax highlighting behavior, including:
- Language-specific lexers
- Formatter selection
- Theme customization
- Line numbers and wrapping controls
This aligns perfectly with the PR objective of enhancing terminal output with colorization.
|
These changes were released in v1.148.0. |
what
Colorize output when we attach TTY
Before:
After:
references
Summary by CodeRabbit
New Features
Documentation
Chores