feat: additional atmos docs parameters for specifying width, using auto-styling and color profile, and preserving new lines
#757
Conversation
additional parameter for specifying the width of markdown outputs, and maintain a consistent color profile
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
cmd/docs.go(5 hunks)
🔇 Additional comments (3)
cmd/docs.go (3)
11-11: LGTM: Clean import and variable declaration
The new lipgloss import and width variable are appropriately declared for the enhanced functionality.
Also applies to: 21-21
50-50: LGTM: Improved error message clarity
The consolidated error message provides clear context about the component and its expected location.
67-77: LGTM: Well-structured renderer configuration
The new glamour renderer configuration properly implements all required features with appropriate error handling.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
go.mod (1)
Line range hint
3-3: Invalid Go version specifiedThe Go version
1.23specified in thego.modfile is not valid. The latest stable version of Go is 1.22.Apply this change:
-go 1.23 +go 1.22
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
cmd/docs.go(5 hunks)go.mod(1 hunks)
🔇 Additional comments (5)
go.mod (1)
44-44: LGTM: Appropriate dependency addition for terminal operations
The addition of golang.org/x/term as a direct dependency is appropriate for implementing the terminal width customization feature. This is a well-maintained package from the Go ecosystem.
cmd/docs.go (4)
11-13: Appropriate Addition of Imports for New Functionality
The inclusion of lipgloss and golang.org/x/term packages is necessary to support color profile handling and terminal size detection.
22-23: Declaration of 'width' Variable
Introducing the width variable enables dynamic word-wrap configuration based on terminal width.
44-61: Implementation of Dynamic Terminal Width Detection
The logic for detecting terminal width when the width flag is not set is well-implemented, with appropriate error handling and sensible defaults.
86-95: Enhanced Glamour Renderer with Additional Styling Options
The updated renderer configuration improves the documentation output by applying color profiles, auto-styling, preserving new lines, and dynamically wrapping text based on the width variable.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
cmd/docs.go(5 hunks)
🧰 Additional context used
📓 Learnings (1)
cmd/docs.go (1)
Learnt from: RoseSecurity
PR: cloudposse/atmos#757
File: cmd/docs.go:52-54
Timestamp: 2024-11-01T14:45:32.417Z
Learning: In `cmd/docs.go`, capping the terminal width at 120 columns is considered acceptable and preferred after testing.
🔇 Additional comments (4)
cmd/docs.go (4)
44-60: LGTM! Robust terminal width detection implementation
The implementation properly handles terminal detection with appropriate fallbacks, following the recommended approach from glow.
52-54: LGTM! Width capping is appropriate
The 120-column cap has been tested and confirmed as an acceptable value.
69-69: LGTM! Clear error message
The error message is concise and informative.
86-96: LGTM! Comprehensive renderer configuration
The glamour renderer is well-configured with appropriate options for color profile, styling, and formatting. However, let's verify the renderer's behavior with different terminal types.
✅ Verification successful
Let me gather more context about the terminal handling in the codebase.
Terminal handling is properly implemented
The code correctly handles terminal detection and width adjustment:
- Checks if output is to a terminal using
term.IsTerminal - Dynamically adjusts width based on terminal size with
term.GetSize - Falls back gracefully when terminal detection fails
- Configures glamour renderer with appropriate terminal-aware settings (color profile, auto-style)
- Implements sensible width limits (capped at 120)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the code handles different terminal types appropriately
# Test: Search for terminal-specific handling
rg -l "term.IsTerminal|term.GetSize|glamour.WithColorProfile|glamour.WithAutoStyle"
Length of output: 97
Script:
#!/bin/bash
# Get the full context of terminal-related code in docs.go
rg -B 5 -A 5 "term.IsTerminal|term.GetSize|glamour.WithColorProfile|glamour.WithAutoStyle" cmd/docs.go
Length of output: 783
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
cmd/docs.go(4 hunks)
🧰 Additional context used
📓 Learnings (1)
cmd/docs.go (1)
Learnt from: RoseSecurity
PR: cloudposse/atmos#757
File: cmd/docs.go:52-54
Timestamp: 2024-11-01T14:45:32.417Z
Learning: In `cmd/docs.go`, capping the terminal width at 120 columns is considered acceptable and preferred after testing.
🔇 Additional comments (3)
cmd/docs.go (3)
11-11: LGTM: Import changes are appropriate.
The new imports for terminal detection (term) and styling (lipgloss) are correctly added and used in the implementation.
Also applies to: 13-13
68-68: LGTM: Error message formatting is improved.
The simplified error message formatting maintains clarity while reducing code complexity.
125-125: LGTM: Width flag implementation.
The flag implementation is complete with a clear description and appropriate default value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pkg/schema/schema.go (1)
35-37: Consider adding field documentation.The
Docsstruct implementation looks good! Consider adding a comment to document the purpose and valid range ofMaxWidth.type Docs struct { + // MaxWidth specifies the maximum width for markdown output rendering. + // Valid range: 1-120, defaults to 80 if not specified. MaxWidth int `yaml:"max-width" json:"max_width" mapstructure:"max-width"` }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
cmd/docs.go(3 hunks)go.mod(1 hunks)pkg/schema/schema.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/docs.go
- go.mod
🔇 Additional comments (1)
pkg/schema/schema.go (1)
32-33: LGTM! Clean addition of Docs field.
The field is properly integrated into the CliSettings struct with consistent tag naming and proper use of omitempty.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cmd/docs.go (1)
42-59: Consider using more descriptive variable names.While the logic is solid, variable names like
wcould be more descriptive. Consider renaming for better clarity:- w, _, err := term.GetSize(int(os.Stdout.Fd())) - if err == nil && w > 0 { - screenWidth = w + termWidth, _, err := term.GetSize(int(os.Stdout.Fd())) + if err == nil && termWidth > 0 { + screenWidth = termWidth
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
cmd/docs.go(3 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
📓 Learnings (1)
cmd/docs.go (1)
Learnt from: RoseSecurity
PR: cloudposse/atmos#757
File: cmd/docs.go:42-59
Timestamp: 2024-11-01T15:44:12.617Z
Learning: In `cmd/docs.go`, when implementing width detection for the `docsCmd` command, it's acceptable to keep the code inline without extracting it into a separate function, as per the user's preference for compact readability and maintainability in Go code.
🔇 Additional comments (4)
cmd/docs.go (4)
11-11: LGTM! Required imports added correctly.
The new imports for lipgloss and term packages are necessary for color profile management and terminal size detection respectively.
Also applies to: 13-13
42-59: LGTM! Width detection logic implemented correctly.
The implementation follows the agreed approach:
- Uses
maxWidthfrom CLI config - Detects terminal width if available
- Applies appropriate fallbacks
- Handles edge cases properly
68-68: LGTM! Clear and informative error message.
The error message provides good context by including both the component name and the path.
85-95: LGTM! Comprehensive renderer configuration.
The term renderer is well-configured with:
- Color profile management
- Auto-styling
- New line preservation
- Word wrapping
- Enhanced error handling
|
Unfriendly error message: (this is more debug log level) Component folder does not exist. We should instead say, "Component vpc not found" Good error message: (no 
|
|
It does not appear to be detecting the full width of my terminal. Maybe we should add some debug messages to see what the max width is determined as and what width is selected. 
Note, the screen is much wider, and the text is not wrapping and is truncated. |
Just removed the logic that defaults back to |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cmd/docs.go (1)
84-94: Consider enhancing the renderer error message.While the renderer configuration is solid, the error message could be more specific about which option caused the failure.
- u.LogErrorAndExit(schema.CliConfiguration{}, fmt.Errorf("failed to initialize markdown renderer: %w", err)) + u.LogErrorAndExit(schema.CliConfiguration{}, fmt.Errorf("failed to initialize markdown renderer with width %d: %w", screenWidth, err))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
cmd/docs.go(3 hunks)
🧰 Additional context used
📓 Learnings (1)
cmd/docs.go (1)
Learnt from: RoseSecurity
PR: cloudposse/atmos#757
File: cmd/docs.go:42-59
Timestamp: 2024-11-01T15:44:12.617Z
Learning: In `cmd/docs.go`, when implementing width detection for the `docsCmd` command, it's acceptable to keep the code inline without extracting it into a separate function, as per the user's preference for compact readability and maintainability in Go code.
🔇 Additional comments (3)
cmd/docs.go (3)
11-11: LGTM! Imports are well-organized.
The new imports for lipgloss and term packages are correctly placed and support the enhanced terminal rendering features.
Also applies to: 13-13
42-59: Well implemented terminal width detection!
The implementation aligns perfectly with the discussed approach:
- Respects user-configured maxWidth from atmos.yaml
- Gracefully falls back to terminal width when available
- Uses sensible defaults (120 characters)
- Maintains clean, compact code structure
67-67: Error messages are now more user-friendly!
The improved error messages clearly indicate the specific issue:
- Component not found message includes the searched path
- Separate message for missing README vs component not found
Also applies to: 75-75
|
It appears there's some fudge factor needed with the width calculation. It's off by 2 characters. I'm wondering if it's related to something like
Maybe this is what you're referring to. But this is a bug, not a result of limiting the width. That causes an extra line feed between each row. If I expand my screen by 2 characters, I get Which is clean. |
characters which appears to be a subtle padding effect at the terminal boundaries
Thanks for the great find! Interesting that there is a subtle padding effect at the terminal boundaries like that. I adjusted accordingly and added a comment for the width subtraction in the code! |
|
Perfect, LGTM! 
|
|
These changes were released in v1.103.0. |
|
Regarding the incorrect screen width calculations (which we resolved with a hack), it might be related to this recently opened issue. |
@osterman It has been resolved in bubbletea 1.2.1, now the issue is still open there because there is something else. See my last comment on that issue to get more insight. |
what
atmos docsflag for specifying the width of markdown outputwhy
widthparameter is useful for users who prefer seeing wider output for Terraform docs-generated tables and is defined in theatmos.yaml:future development
references
glowdocsSummary by CodeRabbit
New Features
Bug Fixes
Documentation