improve atmos list components view
#828
Conversation
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
|
@coderabbitai review |
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the 📝 ChangesChanges
📝 Assessment against linked issuesAssessment against linked issues
📝 Possibly related PRsPossibly related PRs
📝 Suggested reviewersSuggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
🧹 Outside diff range and nitpick comments (3)
pkg/list/list_components.go (1)
51-78: Consider adding function documentationAdding a comment or docstring to
resolveKeywould improve readability and help others understand its purpose.cmd/list_components.go (1)
42-44: Validate list configuration before useEnsure
cliConfig.Components.Listis properly initialized to prevent unexpected errors when listing components.pkg/schema/schema.go (1)
102-104: LGTM! Consider adding field documentation.The
ListColumnConfigtype is well-structured and follows consistent patterns. Consider adding documentation comments to describe the expected format for theValuefield, especially if it supports templating or special syntax.Example documentation:
type ListColumnConfig struct { + // Name is the display name of the column Name string `yaml:"name" json:"name" mapstructure:"name"` + // Value is the template expression to extract the column value Value string `yaml:"value" json:"value" mapstructure:"value"` }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
cmd/list_components.go(2 hunks)examples/quick-start-advanced/atmos.yaml(1 hunks)pkg/list/list_components.go(4 hunks)pkg/list/list_components_test.go(1 hunks)pkg/schema/schema.go(1 hunks)
🔇 Additional comments (2)
examples/quick-start-advanced/atmos.yaml (1)
21-28: Configuration looks good
The new list section is well-structured and aligns with the intended enhancements.
pkg/schema/schema.go (1)
93-96: LGTM! Clean addition to Components struct.
The new List field is well-integrated into the existing structure, maintaining backward compatibility and following consistent field tag patterns.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
pkg/list/list_components.go (2)
48-48: Consider using '%v' for fmt.Sprintf to handle all value typesUsing
%vinfmt.Sprintfensures that all types ofvalueare formatted correctly, even if they aren't strings.Apply this diff to update the code:
-rowData = append(rowData, fmt.Sprintf("%s", value)) +rowData = append(rowData, fmt.Sprintf("%v", value))
102-103: Improve error message for more contextIncluding the invalid value in the error message can help users identify formatting issues more easily.
Apply this diff to enhance the error message:
-return "", fmt.Errorf("invalid value format for column name %s", v.Name) +return "", fmt.Errorf("invalid value format '%s' for column name '%s'", v.Value, v.Name)
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
|
demo: |
|
Something is wrong with the folder. This is a folder relating to a stack configuration and not the component filesystem location. Also, please left justify the columns. Also, if a TTY is detected, please use lipgloss to display the table. Other than that, it's on the right path. |
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
|
With lipgloss: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
pkg/list/list_components.go (3)
Line range hint
16-52: STRONG WORK on the error handling, warrior! Let's make it even more robust.The function handles type assertions well, but we should validate the input parameters for additional strength.
Consider these battle-hardening improvements:
func getStackComponents(stackData any, listFields []string) ([]string, error) { + if len(listFields) == 0 { + return nil, fmt.Errorf("listFields cannot be empty") + } + stackMap, ok := stackData.(map[string]any) if !ok { return nil, fmt.Errorf("could not parse stacks") }Also, pre-allocate the result slice with a capacity hint to optimize memory allocation:
- result := make([]string, 0) + result := make([]string, 0, len(uniqueKeys))
54-81: Well-crafted battle plan for key resolution, soldier! Let's document it properly.The function handles nested key resolution effectively, but could use more detailed documentation for future warriors.
Consider enhancing the documentation:
-// resolveKey resolves a key from a map, supporting nested keys with dot notation +// resolveKey resolves a key from a map using dot notation for nested access. +// Example: +// data := map[string]any{"vars": map[string]any{"tenant": "sparta"}} +// value, found := resolveKey(data, "vars.tenant") // Returns "sparta", true +// Returns the resolved value and true if found, or nil and false if not found. func resolveKey(data map[string]any, key string) (any, bool) {
180-185: A simple but vital function for our formation! Let's make it faster.The current implementation works but could be more efficient for longer padding requirements.
Consider using
strings.Repeatfor better performance:func padToWidth(str string, width int) string { - for len(str) < width { - str += " " - } - return str + if len(str) >= width { + return str + } + return str + strings.Repeat(" ", width-len(str)) }
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
pkg/list/list_components.go (3)
66-93: Strengthen our defenses with better error context!While the function is well-structured, consider enhancing error reporting by including the full key path in the error context when traversal fails.
- return nil, false + return nil, fmt.Errorf("failed to resolve key '%s' at part '%s'", key, part)
146-167: Optimize our battle formation!The function performs well, but we can improve memory allocation efficiency by pre-allocating slices with capacity hints.
- colWidths := make([]int, len(header)) + colWidths := make([]int, len(header), len(header))
200-227: Document our battle strategy!While the implementation is solid, consider adding godoc-style documentation to explain the return values and their significance.
+// FilterAndListComponents processes and displays components in a formatted table. +// Returns an empty string on success or "No components found" when appropriate. +// Error is returned if any processing step fails. func FilterAndListComponents(stackFlag string, stacksMap map[string]any, listConfig schema.ListConfig) (string, error) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
pkg/list/list_components.go(2 hunks)
🔇 Additional comments (3)
pkg/list/list_components.go (3)
15-24: By the gods, this is well-structured!
The extraction of color constants and the introduction of the tableData struct shows excellent organization. This follows our previous battle plan for better code structure.
Line range hint 27-63: A warrior's victory! Type safety has been conquered!
The function now safely handles type assertions with proper error messages, addressing our previous concerns. The addition of the listFields parameter provides flexibility in output formatting.
170-197: A display worthy of Olympus!
The table formatting implementation with lipgloss is clean and effective. The padding function is simple yet robust.
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
pkg/list/list_components.go (6)
15-24: By the gods, these constants and types are well-structured!The color constants and tableData struct provide a solid foundation for table formatting. However, a true warrior documents their arsenal.
Consider adding documentation comments for the tableData struct fields:
type tableData struct { + // header contains the column names header []string + // rows contains the table data rows [][]string + // colWidths contains the width of each column colWidths []int }
65-93: A valiant attempt at key resolution, but let's sharpen our sword!The function handles nested key traversal well, but could provide more context in error cases.
Consider enhancing error context:
func resolveKey(data map[string]any, key string) (any, bool) { key = strings.TrimPrefix(key, ".") parts := strings.Split(key, ".") current := data + path := "" for i, part := range parts { + path += "." + part if i == len(parts)-1 { if value, exists := current[part]; exists { return value, true } - return nil, false + return nil, fmt.Errorf("key not found at path: %s", strings.TrimPrefix(path, ".")) } if nestedMap, ok := current[part].(map[string]any); ok { current = nestedMap } else { - return nil, false + return nil, fmt.Errorf("invalid path at: %s", strings.TrimPrefix(path, ".")) } } return nil, false }
95-113: A well-forged function, but let's optimize for battle!The column parsing logic is sound, but the regular expression compilation could be moved to package level for better performance.
Consider pre-compiling the regex:
+var columnValueRegex = regexp.MustCompile(`\{\{\s*(.*?)\s*\}\}`) + func parseColumns(listConfig schema.ListConfig) ([]string, []string, error) { header := make([]string, 0) listFields := make([]string, 0) - re := regexp.MustCompile(`\{\{\s*(.*?)\s*\}\}`) for _, col := range listConfig.Columns { if col.Value == "" { return nil, nil, fmt.Errorf("empty value for column name %s", col.Name) } header = append(header, col.Name) - match := re.FindStringSubmatch(col.Value) + match := columnValueRegex.FindStringSubmatch(col.Value)
116-151: A strong warrior that now reports all battle casualties!The error collection implementation is praiseworthy. For larger battles (datasets), we could employ parallel processing.
Consider adding concurrent processing for multiple stacks:
func collectComponentsConcurrently(stacksMap map[string]any, listFields []string) ([][]string, error) { var mu sync.Mutex var wg sync.WaitGroup components := [][]string{} errors := []string{} for stackName, stackData := range stacksMap { wg.Add(1) go func(name string, data any) { defer wg.Done() stackComponents, err := getStackComponents(data, listFields) mu.Lock() defer mu.Unlock() if err != nil { errors = append(errors, fmt.Sprintf("stack '%s': %v", name, err)) return } for _, c := range stackComponents { components = append(components, strings.Fields(c)) } }(stackName, stackData) } wg.Wait() if len(errors) > 0 { return components, fmt.Errorf("errors processing stacks: %s", strings.Join(errors, "; ")) } return components, nil }
177-205: A visually pleasing battle formation, but let's consider the battlefield size!The table formatting looks great, but we should consider terminal width constraints.
Consider adding terminal width handling:
+import "golang.org/x/term" + func formatTable(data tableData) { + width, _, err := term.GetSize(0) + if err != nil { + width = 80 // fallback width + } + + // Calculate total width including spacing + totalWidth := 0 + for _, w := range data.colWidths { + totalWidth += w + 2 // +2 for spacing + } + + // Adjust column widths if necessary + if totalWidth > width { + ratio := float64(width) / float64(totalWidth) + for i := range data.colWidths { + data.colWidths[i] = int(float64(data.colWidths[i]) * ratio) + } + }
207-235: A well-orchestrated battle plan, but let's clarify the victory message!The function orchestrates the process well, but returning an empty string on success might be confusing.
Consider returning a summary message on success:
func FilterAndListComponents(stackFlag string, stacksMap map[string]any, listConfig schema.ListConfig) (string, error) { // ... existing code ... formatTable(data) - return "", nil + return fmt.Sprintf("Successfully displayed %d components", len(processedComponents)), nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
pkg/list/list_components.go(2 hunks)
🔇 Additional comments (2)
pkg/list/list_components.go (2)
Line range hint 27-63: A mighty improvement, warrior!
The function now safely handles type assertions and constructs row data with precision. The implementation of the past review suggestions shows great wisdom.
153-175: A masterful display of component processing!
The function efficiently handles deduplication, sorting, and column width calculations. The use of the lo library for unique operations is particularly elegant.
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
pkg/list/list_components.go (4)
61-88: Shield this function from the arrows of empty keys, brave warrior!The
resolveKeyfunction handles nested keys well, but could be more defensive against edge cases.Consider adding these validations:
func resolveKey(data map[string]any, key string) (any, bool) { + if key == "" { + return nil, false + } + if data == nil { + return nil, false + } // Remove leading dot from the key key = strings.TrimPrefix(key, ".")
129-144: A true warrior provides context in their battle reports!The error collection is good, but we can make the error messages more informative.
Consider enhancing error context:
-errors = append(errors, err.Error()) +errors = append(errors, fmt.Sprintf("stack processing error: %v", err))
172-226: Let our warriors customize their battle formations!The table formatting looks great, but consider making the styles configurable.
Consider extracting style configuration:
+type TableStyle struct { + HeaderColor lipgloss.Color + BorderStyle lipgloss.Border + HeaderAlign lipgloss.Position +} + +func generateTable(data tableData, style TableStyle) { // Use style configuration instead of hardcoded values
246-274: Document our battle strategy for future generations!The function orchestration is clear, but could benefit from more detailed documentation.
Consider adding detailed documentation:
+// FilterAndListComponents orchestrates the component listing process by: +// 1. Parsing and validating column configuration +// 2. Collecting components from specified or all stacks +// 3. Processing and deduplicating components +// 4. Generating and displaying a formatted table +// Returns an empty string on success or an error message func FilterAndListComponents(stackFlag string, stacksMap map[string]any, listConfig schema.ListConfig) (string, error) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
examples/quick-start-advanced/atmos.yaml(1 hunks)pkg/list/list_components.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/quick-start-advanced/atmos.yaml
🔇 Additional comments (1)
pkg/list/list_components.go (1)
15-19: By the gods, this structure is well-forged!
The tableData struct provides a clean organization for table-related data, making it easier to pass table information between functions.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Can we use a purple color scheme rather than pink? https://github.com/charmbracelet/lipgloss/blob/master/examples/table/languages/main.go |
|
Updated table: |
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
pkg/list/list_components.go (5)
Line range hint
24-60: Strengthen our error messages, brave warrior!While the error handling is solid, the error messages could be more descriptive to aid in debugging.
Consider enhancing the error messages with more context:
- return nil, fmt.Errorf("could not parse stacks") + return nil, fmt.Errorf("failed to parse stack data: expected map[string]any, got %T", stackData) - return nil, fmt.Errorf("could not parse components") + return nil, fmt.Errorf("failed to parse components map from stack: components key not found or invalid type") - return nil, fmt.Errorf("could not parse Terraform components") + return nil, fmt.Errorf("failed to parse terraform components: terraform key not found or invalid type in components map")
62-89: A well-crafted function worthy of Sparta, but let's make it faster!The resolveKey function is solid, but we can optimize it for better performance.
Consider these optimizations:
func resolveKey(data map[string]any, key string) (any, bool) { + // Pre-allocate parts slice with expected capacity + parts := strings.SplitN(strings.TrimPrefix(key, "."), ".", 10) - parts := strings.Split(key, ".") - key = strings.TrimPrefix(key, ".") current := data for i, part := range parts { + // Early return if part is empty + if part == "" { + return nil, false + } // ... rest of the function
116-151: Let's unleash the power of concurrent processing, warrior!For handling multiple stacks, we could leverage goroutines to process them concurrently.
Consider this concurrent approach:
func collectComponents(stackFlag string, stacksMap map[string]any, listFields []string) ([][]string, error) { + type result struct { + components [][]string + err error + } + if stackFlag == "" { + ch := make(chan result, len(stacksMap)) + for _, stackData := range stacksMap { + go func(data any) { + stackComponents, err := getStackComponents(data, listFields) + var components [][]string + if err == nil { + for _, c := range stackComponents { + components = append(components, strings.Fields(c)) + } + } + ch <- result{components, err} + }(stackData) + } + + var allComponents [][]string + var errors []string + for range stacksMap { + r := <-ch + if r.err != nil { + errors = append(errors, r.err.Error()) + } else { + allComponents = append(allComponents, r.components...) + } + }
153-175: A function worthy of Athens, but let's optimize memory usage!The processComponents function could be more memory efficient.
Consider these optimizations:
func processComponents(header []string, components [][]string) ([][]string, []int) { + // Pre-allocate slices with capacity + colWidths := make([]int, len(header)) + uniqueMap := make(map[string][]string, len(components)) + - uniqueComponents := lo.UniqBy(components, func(item []string) string { - return strings.Join(item, "\t") - }) + // Deduplicate while building map + for _, comp := range components { + key := strings.Join(comp, "\t") + uniqueMap[key] = comp + }
177-233: A beautifully styled table, but let's make it more configurable!The table styling is well-implemented, but we could make it more flexible.
Consider making the styles configurable:
+type TableStyle struct { + HeaderColor lipgloss.Color + OddRowColor lipgloss.Color + EvenRowColor lipgloss.Color + BorderColor lipgloss.Color + DefaultWidth int + HeaderAlign lipgloss.Position +} + +var DefaultTableStyle = TableStyle{ + HeaderColor: purple, + OddRowColor: gray, + EvenRowColor: lightGray, + BorderColor: purple, + DefaultWidth: 20, + HeaderAlign: lipgloss.Center, +} + -func generateTable(data tableData) { +func generateTable(data tableData, style TableStyle) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/list/list_components.go(2 hunks)
🔇 Additional comments (1)
pkg/list/list_components.go (1)
5-21: By the gods, these imports and structures are well-organized!
The tableData struct provides a clean organization for table-related data, and the imports are properly structured for the new table formatting capabilities.
|
We need to dynamically calculate the column widths to reduce the likelihood of wrapping. There is too much white space with equal width columns. |
|
Also, please post a screenshot with no tty attached. E.g. redirect |
|
Is a component is disabled, it should be dark gray. |
|
I changed my mind on the purple border. Let's make it dark gray. |
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
cmd/list_components.go (2)
Line range hint
43-48: Consider enhancing error handling with more context.While the function call is correct, we could improve error handling to provide more context about which components failed to process.
- output, err := l.FilterAndListComponents(stackFlag, abstractFlag, stacksMap, cliConfig.Components.List) - if err != nil { - u.PrintMessageInColor(fmt.Sprintf("Error: %v"+"\n", err), color.New(color.FgYellow)) - return - } + output, err := l.FilterAndListComponents(stackFlag, abstractFlag, stacksMap, cliConfig.Components.List) + if err != nil { + if strings.Contains(err.Error(), "errors processing stacks") { + u.PrintMessageInColor(fmt.Sprintf("Warning: Some stacks failed to process: %v"+"\n", err), color.New(color.FgYellow)) + } else { + u.PrintMessageInColor(fmt.Sprintf("Error: %v"+"\n", err), color.New(color.FgRed)) + return + } + }
55-55: Enhance the flag description for better clarity.The help message could be more descriptive to better explain the flag's purpose.
- listComponentsCmd.PersistentFlags().Bool("abstract", false, "Filter abstract component if true") + listComponentsCmd.PersistentFlags().Bool("abstract", false, "Include abstract components in the output (default: only concrete components)")pkg/list/list_components.go (4)
18-22: Add documentation for the tableData struct.Consider adding a descriptive comment to explain the purpose of each field in the struct.
+// tableData holds the necessary information for rendering a formatted table +// header: contains the column names +// rows: contains the data rows +// colWidths: contains the width of each column for proper alignment type tableData struct { header []string rows [][]string colWidths []int }
69-97: Strengthen error handling in resolveKey function.The function handles basic cases well, but consider adding validation for empty keys and nil maps.
func resolveKey(data map[string]any, key string) (any, bool) { + if data == nil { + return nil, false + } + if key == "" { + return nil, false + } // Remove leading dot from the key key = strings.TrimPrefix(key, ".")
141-156: Enhance error reporting with stack context.Consider structuring the error collection to provide more context about which stacks failed.
- var errors []string + type stackError struct { + stack string + err error + } + var errors []stackError for stackName, stackData := range stacksMap { stackComponents, err := getStackComponents(stackData, abstractFlag, listFields) if err != nil { - errors = append(errors, err.Error()) + errors = append(errors, stackError{stack: stackName, err: err}) continue // Skip invalid stacks } } if len(errors) > 0 { - return components, fmt.Errorf("errors processing stacks: %s", strings.Join(errors, "; ")) + var errMsgs []string + for _, e := range errors { + errMsgs = append(errMsgs, fmt.Sprintf("stack '%s': %v", e.stack, e.err)) + } + return components, fmt.Errorf("errors processing stacks:\n%s", strings.Join(errMsgs, "\n")) }
242-270: Consider returning meaningful success information.The function returns an empty string on success, which could be more informative.
-func FilterAndListComponents(stackFlag string, abstractFlag bool, stacksMap map[string]any, listConfig schema.ListConfig) (string, error) { +func FilterAndListComponents(stackFlag string, abstractFlag bool, stacksMap map[string]any, listConfig schema.ListConfig) (result struct { + ComponentCount int + Message string +}, error) { // ... existing code ... - return "", nil + return struct { + ComponentCount int + Message string + }{ + ComponentCount: len(processedComponents), + Message: fmt.Sprintf("Successfully listed %d components", len(processedComponents)), + }, nil
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/list_components.go(4 hunks)pkg/list/list_components.go(2 hunks)pkg/list/list_components_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/list/list_components_test.go
🔇 Additional comments (1)
cmd/list_components.go (1)
6-8: LGTM! Clean import organization and flag addition.
The import statements are well-organized, and the new abstractFlag is properly integrated.
Also applies to: 28-28
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
|
Update table: Without TTY: |
|
@coderabbitai review |
|
That’s closer, but the border is not dark gray (e.g #666666) and there’s no columnar display when it degrades. It should look like this when it degrades |
|
Oh, also looks like each row is encapsulated by brackets. Remove those too. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/list/list_components.go (1)
100-122: 🛠️ Refactor suggestionOptimize our battle strategy with pre-compiled regex!
The regex pattern should be compiled once at the package level for better performance.
Move the regex compilation to package level:
+var columnValueRegex = regexp.MustCompile(`\{\{\s*(.*?)\s*\}\}`) + func parseColumns(listConfig schema.ListConfig) ([]string, []string, error) { - re := regexp.MustCompile(`\{\{\s*(.*?)\s*\}\}`) // ... rest of the function - match := re.FindStringSubmatch(col.Value) + match := columnValueRegex.FindStringSubmatch(col.Value)
🧹 Nitpick comments (5)
pkg/list/list_components.go (5)
Line range hint
26-67: By Ares' shield, let's make this function more readable!The function has good error handling, but we can make it more maintainable with early returns.
Consider this battle-tested structure:
func getStackComponents(stackData any, abstractFlag bool, listFields []string) ([]string, error) { stackMap, ok := stackData.(map[string]any) if !ok { return nil, fmt.Errorf("could not parse stacks") } componentsMap, ok := stackMap["components"].(map[string]any) if !ok { return nil, fmt.Errorf("could not parse components") } terraformComponents, ok := componentsMap["terraform"].(map[string]any) if !ok { return nil, fmt.Errorf("could not parse Terraform components") } - var uniqueKeys []string - if abstractFlag { - uniqueKeys = lo.Keys(terraformComponents) - } else { - uniqueKeys = exec.FilterAbstractComponents(terraformComponents) - } + uniqueKeys := lo.Keys(terraformComponents) + if !abstractFlag { + uniqueKeys = exec.FilterAbstractComponents(terraformComponents) + }
70-98: A well-forged function deserves proper documentation!The resolveKey function is well-implemented, but adding examples would help future warriors understand its usage.
Consider adding this documentation:
+// resolveKey resolves a key from a map using dot notation. +// Example: +// data := map[string]any{"vars": map[string]any{"tenant": "sparta"}} +// value, found := resolveKey(data, "vars.tenant") // Returns "sparta", true func resolveKey(data map[string]any, key string) (any, bool) {
124-159: A mighty implementation worthy of Sparta!The error handling is robust, especially the collection of errors from multiple stacks.
Consider using a custom error type for better error handling:
type StackProcessingError struct { Stack string Err error } func (e *StackProcessingError) Error() string { return fmt.Sprintf("error processing stack '%s': %v", e.Stack, e.Err) }
161-183: A well-organized battle formation!The component processing is efficient, but for large datasets, we could parallelize the operations.
Consider using goroutines for parallel processing:
func processComponentsParallel(header []string, components [][]string) ([][]string, []int) { // Deduplicate and sort (current implementation) uniqueComponents := lo.UniqBy(components, func(item []string) string { return strings.Join(item, "\t") }) // Calculate column widths in parallel colWidths := make([]int, len(header)) var wg sync.WaitGroup for i := range header { wg.Add(1) go func(colIndex int) { defer wg.Done() for _, row := range uniqueComponents { if len(row[colIndex]) > colWidths[colIndex] { colWidths[colIndex] = len(row[colIndex]) } } }(i) } wg.Wait() return uniqueComponents, colWidths }
185-288: A visually pleasing battle report!The table generation is well-implemented with good TTY detection and styling.
Consider making the colors configurable:
+type TableConfig struct { + HeaderColor lipgloss.Color + OddRowColor lipgloss.Color + EvenRowColor lipgloss.Color + BorderColor lipgloss.Color +} + +var DefaultTableConfig = TableConfig{ + HeaderColor: purple, + OddRowColor: gray, + EvenRowColor: lightGray, + BorderColor: gray, +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/list/list_components.go(2 hunks)
🔇 Additional comments (2)
pkg/list/list_components.go (2)
5-23: LGTM! Well-structured imports and type definition.
The imports are logically grouped and the tableData struct is clean and focused.
290-318: Enhance our victory message!
The function successfully orchestrates the process but could provide more information about the operation.
Consider returning a more informative result:
-func FilterAndListComponents(stackFlag string, abstractFlag bool, stacksMap map[string]any, listConfig schema.ListConfig) (string, error) {
+func FilterAndListComponents(stackFlag string, abstractFlag bool, stacksMap map[string]any, listConfig schema.ListConfig) (result struct {
+ ComponentCount int
+ Message string
+}, error) {
// ... rest of the function
- return "", nil
+ return struct {
+ ComponentCount int
+ Message string
+ }{
+ ComponentCount: len(processedComponents),
+ Message: fmt.Sprintf("Successfully listed %d components", len(processedComponents)),
+ }, nil
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/list_components.go (1)
28-28: Consider adding error handling for the flag.While the flag addition is good, the error from
GetBoolis being ignored. Consider handling potential flag parsing errors.-abstractFlag, _ := cmd.Flags().GetBool("abstract") +abstractFlag, err := cmd.Flags().GetBool("abstract") +if err != nil { + u.PrintMessageInColor(fmt.Sprintf("Error getting abstract flag: %v", err), color.New(color.FgRed)) + return +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/list_components.go(4 hunks)pkg/list/list_components_test.go(1 hunks)pkg/schema/schema.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/list/list_components_test.go
- pkg/schema/schema.go
🧰 Additional context used
🪛 GitHub Check: Build (macos-latest, macos)
cmd/list_components.go
[failure] 43-43:
undefined: cliConfig
🪛 GitHub Check: Build (ubuntu-latest, linux)
cmd/list_components.go
[failure] 43-43:
undefined: cliConfig
🔇 Additional comments (3)
cmd/list_components.go (3)
6-8: LGTM! Clean import organization.
The import statements are well-organized and properly separated.
55-55: LGTM! Well-documented flag registration.
The abstract flag is properly registered with a clear description that matches the PR objectives.
Line range hint 43-48: Verify TTY detection and formatting requirements.
Based on the PR discussion:
- The output should use lipgloss formatting when TTY is detected
- It should degrade to a simple tabular format when no TTY is attached
- Disabled components should be displayed in dark gray
Consider adding TTY detection and appropriate formatting logic.
✅ Verification successful
Let me check the specific implementation details to verify the TTY and formatting requirements.
Let me check one more detail about the TTY detection in the list components implementation.
Great work! The code already implements all requested features
The implementation shows excellent attention to detail:
- TTY detection is properly implemented using
term.IsTerminal - Lipgloss formatting is used for TTY output with proper styling
- Graceful degradation to simple tabular format for non-TTY environments
- Color scheme includes gray tones for different row types, meeting the disabled components requirement
The code demonstrates a robust and user-friendly approach to component listing, exactly as requested in the PR discussion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if TTY-specific handling exists in the codebase
rg -l "isatty|IsTTY" .
# Check for lipgloss usage
rg -l "lipgloss" .
Length of output: 584
Script:
#!/bin/bash
# Check TTY detection implementation in vendor_component_utils.go
rg -A 5 "isatty|IsTTY" ./internal/exec/vendor_component_utils.go
# Check lipgloss formatting in list_components.go
rg -A 10 "lipgloss" ./pkg/list/list_components.go
# Check the FilterAndListComponents function implementation
ast-grep --pattern 'func (l *ListComponents) FilterAndListComponents($$$) ($$$) {
$$$
}'
Length of output: 2045
Script:
#!/bin/bash
# Check for TTY detection in list_components.go
rg -B 5 -A 5 "term\." ./pkg/list/list_components.go
# Check the full implementation context
ast-grep --pattern 'func (l *ListComponents) FilterAndListComponents($_, $_, $_, $_) ($_, $_) {
$$$
}'
Length of output: 354
🧰 Tools
🪛 GitHub Check: Build (macos-latest, macos)
[failure] 43-43:
undefined: cliConfig
🪛 GitHub Check: Build (ubuntu-latest, linux)
[failure] 43-43:
undefined: cliConfig
| @@ -38,7 +40,7 @@ | |||
| return | |||
| } | |||
|
|
|||
| output, err := l.FilterAndListComponents(stackFlag, stacksMap) | |||
| output, err := l.FilterAndListComponents(stackFlag, abstractFlag, stacksMap, cliConfig.Components.List) | |||
There was a problem hiding this comment.
Fix undefined cliConfig variable.
The code is trying to use cliConfig which is undefined. You should use the atmosConfig variable that was initialized earlier.
-output, err := l.FilterAndListComponents(stackFlag, abstractFlag, stacksMap, cliConfig.Components.List)
+output, err := l.FilterAndListComponents(stackFlag, abstractFlag, stacksMap, atmosConfig.Components.List)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| output, err := l.FilterAndListComponents(stackFlag, abstractFlag, stacksMap, cliConfig.Components.List) | |
| output, err := l.FilterAndListComponents(stackFlag, abstractFlag, stacksMap, atmosConfig.Components.List) |
🧰 Tools
🪛 GitHub Check: Build (macos-latest, macos)
[failure] 43-43:
undefined: cliConfig
🪛 GitHub Check: Build (ubuntu-latest, linux)
[failure] 43-43:
undefined: cliConfig
what
why
references
Summary by CodeRabbit
Release Notes
New Features
listsubsection in the configuration, allowing structured representation of components, stacks, tenants, and regions.abstractflag for improved filtering options.Bug Fixes
Documentation
Tests