add pager to describe config command #1203
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1203 +/- ##
==========================================
+ Coverage 33.56% 33.77% +0.20%
==========================================
Files 226 229 +3
Lines 24219 24563 +344
==========================================
+ Hits 8130 8295 +165
- Misses 14875 15049 +174
- Partials 1214 1219 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
samtholiya
force-pushed
the
feature/dev-3131-add-pager-to-atmos-describe-config-command
branch
from
6b4f627 to
39dd127
Compare
samtholiya
force-pushed
the
feature/dev-3131-add-pager-to-atmos-describe-config-command
branch
from
39dd127 to
180bc51
Compare
|
TODO:
|
samtholiya
force-pushed
the
feature/dev-3131-add-pager-to-atmos-describe-config-command
branch
2 times, most recently
from
c52536d to
7de12fb
Compare
📝 WalkthroughWalkthroughThis change introduces a native pager UI for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ConfigLoader
participant Pager
participant Terminal
User->>CLI: Run `atmos describe config`
CLI->>ConfigLoader: Load config, parse flags/env
ConfigLoader-->>CLI: Return config, check pager enabled
CLI->>Pager: If pager enabled, call Pager.Run(title, content)
Pager->>Terminal: Render content in Bubble Tea UI
Terminal-->>Pager: User navigation/copy/quit
Pager-->>CLI: Return on exit
CLI->>User: Show output or errors
Assessment against linked issues
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 comments (1)
pkg/pager/pager.go (1)
1-38: 🛠️ Refactor suggestionAdd test coverage for the pager implementation
The static analysis shows no test coverage for this file. Consider adding tests to verify the pager functionality, especially since it's a core user-facing feature.
#!/bin/bash # Check if there are any tests for the pager rg "TestPager|pager.*test" --type go🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 17-20: pkg/pager/pager.go#L17-L20
Added lines #L17 - L20 were not covered by tests
[warning] 23-36: pkg/pager/pager.go#L23-L36
Added lines #L23 - L36 were not covered by tests
🧹 Nitpick comments (9)
pkg/utils/json_utils.go (1)
38-56: Solid implementation of GetAtmosConfigJSON functionThe function properly handles JSON conversion, indentation, and highlighting with appropriate error handling. The two code paths (highlighted or fallback to plain text) are clear and maintainable.
A small improvement would be to add test coverage for this function since static analysis shows it's not currently covered.
#!/bin/bash # Check if there are any tests for this function rg "TestGetAtmosConfigJSON|func.*GetAtmosConfigJSON.*test" --type go🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 38-42: pkg/utils/json_utils.go#L38-L42
Added lines #L38 - L42 were not covered by tests
[warning] 44-48: pkg/utils/json_utils.go#L44-L48
Added lines #L44 - L48 were not covered by tests
[warning] 50-53: pkg/utils/json_utils.go#L50-L53
Added lines #L50 - L53 were not covered by tests
[warning] 55-55: pkg/utils/json_utils.go#L55
Added line #L55 was not covered by testspkg/utils/yaml_utils.go (1)
52-63: GetAtmosConfigYAML implementation is solidThe function follows the same pattern as GetAtmosConfigJSON with proper error handling. The key difference is that on highlighting error, this function returns the original YAML with the error instead of swallowing the error and proceeding with plain text.
Consider adding test coverage for this function.
#!/bin/bash # Check if there are any tests for this function rg "TestGetAtmosConfigYAML|func.*GetAtmosConfigYAML.*test" --type go🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 55-56: pkg/utils/yaml_utils.go#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 60-60: pkg/utils/yaml_utils.go#L60
Added line #L60 was not covered by testscmd/describe_config.go (1)
19-50: Improved error handling with RunE implementationThe change from
RuntoRunEwith explicit error handling for each step is a significant improvement. The code now properly initializes the CLI config, checks flags, and propagates errors.Consider adding tests for this command execution path since static analysis shows no coverage.
#!/bin/bash # Check if there are any tests for this command rg "TestDescribeConfigCmd|describe_config.*test" --type go🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 19-25: cmd/describe_config.go#L19-L25
Added lines #L19 - L25 were not covered by tests
[warning] 27-30: cmd/describe_config.go#L27-L30
Added lines #L27 - L30 were not covered by tests
[warning] 32-35: cmd/describe_config.go#L32-L35
Added lines #L32 - L35 were not covered by tests
[warning] 37-42: cmd/describe_config.go#L37-L42
Added lines #L37 - L42 were not covered by tests
[warning] 45-45: cmd/describe_config.go#L45
Added line #L45 was not covered by tests
[warning] 49-49: cmd/describe_config.go#L49
Added line #L49 was not covered by testspkg/pager/pager.go (1)
23-37: Concise Run implementation with proper error handlingThe Run method is straightforward and handles errors appropriately. The use of tea.WithAltScreen() and tea.WithMouseCellMotion() options provides a good user experience.
Consider adding a context parameter for cancellation support, as UI operations might run for extended periods.
-func (p *pageCreator) Run(title, content string) error { +func (p *pageCreator) Run(ctx context.Context, title, content string) error { if _, err := p.newTeaProgram( &model{ title: title, content: content, ready: false, viewport: viewport.New(0, 0), }, tea.WithAltScreen(), // use the full size of the terminal in its "alternate screen buffer" tea.WithMouseCellMotion(), // turn on mouse support so we can track the mouse wheel + tea.WithContext(ctx), // add context support for cancellation ).Run(); err != nil { return err } return nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-36: pkg/pager/pager.go#L23-L36
Added lines #L23 - L36 were not covered by testsinternal/exec/describe_config_test.go (1)
98-112: Test case name doesn't match implementationThe test case name suggests it's testing query functionality, but it appears to be testing the same scenario as the NoQuery_NoTTY test with an empty query string.
Consider renaming this test case to better reflect what it's actually testing, or update the test to include a meaningful query expression.
pkg/pager/model_test.go (1)
157-160: Error message has a typoThe assertion message states "max(3, 5) should return 3" but it should say "max(3, 5) should return 5".
- assert.Equal(t, 5, result, "max(3, 5) should return 3") + assert.Equal(t, 5, result, "max(3, 5) should return 5")pkg/pager/model.go (1)
54-64: Consider documenting viewport limitationsWhile the implementation is sound, the viewport doesn't reflow content after it's set. This is likely a limitation of the viewport component itself.
Consider adding a brief comment about this limitation for future developers working with this code.
internal/exec/describe_config.go (2)
23-24: Prefererrors.Newfor static error values
fmt.Errorfincurs the (tiny) formatting overhead that is unnecessary for a constant‑string error. Swapping toerrors.Newkeeps the intent clear and is consistent with other static error declarations in the code‑base.-import "fmt" +import ( + "errors" + "fmt" ... -var ErrTTYNotSupported = fmt.Errorf("tty not supported for this command") +var ErrTTYNotSupported = errors.New("tty not supported for this command")
71-83: Exercise the error branches in tests
codecovpoints out that the YAML/JSON conversion error paths (L77‑78, 82‑83) and the pager failure branch (L90‑91) are not covered.
Adding unit tests that stubGetAtmosConfigYAML/JSONandpageCreator.Runto return errors will:
- lock‑in the fallback behaviour you intend, and
- keep coverage above the project threshold.
Happy to help draft the tests if you’d like.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 77-78: internal/exec/describe_config.go#L77-L78
Added lines #L77 - L78 were not covered by tests
[warning] 82-83: internal/exec/describe_config.go#L82-L83
Added lines #L82 - L83 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
cmd/describe.go(1 hunks)cmd/describe_config.go(2 hunks)internal/exec/describe_config.go(1 hunks)internal/exec/describe_config_test.go(1 hunks)pkg/config/load.go(2 hunks)pkg/pager/mock_pager.go(1 hunks)pkg/pager/model.go(1 hunks)pkg/pager/model_test.go(1 hunks)pkg/pager/pager.go(1 hunks)pkg/schema/schema.go(1 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/yaml_utils.go(1 hunks)pkg/utils/yq_utils.go(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/utils/yq_utils.go (2)
pkg/schema/schema.go (1)
AtmosConfiguration(13-46)pkg/utils/yaml_utils.go (2)
ConvertToYAML(89-95)UnmarshalYAML(222-224)
pkg/pager/model.go (2)
internal/tui/components/code_view/main.go (1)
Model(11-16)pkg/pager/pager.go (1)
New(17-21)
🪛 GitHub Check: codecov/patch
pkg/utils/json_utils.go
[warning] 38-42: pkg/utils/json_utils.go#L38-L42
Added lines #L38 - L42 were not covered by tests
[warning] 44-48: pkg/utils/json_utils.go#L44-L48
Added lines #L44 - L48 were not covered by tests
[warning] 50-53: pkg/utils/json_utils.go#L50-L53
Added lines #L50 - L53 were not covered by tests
[warning] 55-55: pkg/utils/json_utils.go#L55
Added line #L55 was not covered by tests
pkg/utils/yq_utils.go
[warning] 82-91: pkg/utils/yq_utils.go#L82-L91
Added lines #L82 - L91 were not covered by tests
[warning] 93-108: pkg/utils/yq_utils.go#L93-L108
Added lines #L93 - L108 were not covered by tests
[warning] 110-113: pkg/utils/yq_utils.go#L110-L113
Added lines #L110 - L113 were not covered by tests
[warning] 115-115: pkg/utils/yq_utils.go#L115
Added line #L115 was not covered by tests
cmd/describe_config.go
[warning] 19-25: cmd/describe_config.go#L19-L25
Added lines #L19 - L25 were not covered by tests
[warning] 27-30: cmd/describe_config.go#L27-L30
Added lines #L27 - L30 were not covered by tests
[warning] 32-35: cmd/describe_config.go#L32-L35
Added lines #L32 - L35 were not covered by tests
[warning] 37-42: cmd/describe_config.go#L37-L42
Added lines #L37 - L42 were not covered by tests
[warning] 45-45: cmd/describe_config.go#L45
Added line #L45 was not covered by tests
[warning] 49-49: cmd/describe_config.go#L49
Added line #L49 was not covered by tests
pkg/pager/pager.go
[warning] 17-20: pkg/pager/pager.go#L17-L20
Added lines #L17 - L20 were not covered by tests
[warning] 23-36: pkg/pager/pager.go#L23-L36
Added lines #L23 - L36 were not covered by tests
pkg/utils/yaml_utils.go
[warning] 55-56: pkg/utils/yaml_utils.go#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 60-60: pkg/utils/yaml_utils.go#L60
Added line #L60 was not covered by tests
internal/exec/describe_config.go
[warning] 77-78: internal/exec/describe_config.go#L77-L78
Added lines #L77 - L78 were not covered by tests
[warning] 82-83: internal/exec/describe_config.go#L82-L83
Added lines #L82 - L83 were not covered by tests
[warning] 90-91: internal/exec/describe_config.go#L90-L91
Added lines #L90 - L91 were not covered by tests
pkg/pager/mock_pager.go
[warning] 25-28: pkg/pager/mock_pager.go#L25-L28
Added lines #L25 - L28 were not covered by tests
[warning] 32-33: pkg/pager/mock_pager.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 37-41: pkg/pager/mock_pager.go#L37-L41
Added lines #L37 - L41 were not covered by tests
[warning] 45-47: pkg/pager/mock_pager.go#L45-L47
Added lines #L45 - L47 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (19)
tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden (1)
63-63: LGTM: Configuration update for pager featureThe addition of
disable_pager: falseis consistent with the PR's objective to implement a pager feature. This default value ensures the pager is enabled by default.tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden (1)
168-168: LGTM: Configuration update for pager featureThe addition of
disable_pager: falseproperly reflects the default state of the pager feature in configuration imports. This maintains consistency with the core configuration structure.tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden (1)
50-50: LGTM: Configuration update for pager featureThe addition of
disable_pager: falsein the YAML output format is consistent with the other configuration snapshots and correctly reflects the default pager behavior.tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden (1)
155-156: Configuration update for pager feature looks good.The new
disable_pagerfield with default valuefalsehas been properly added to the configuration snapshot, maintaining the overall structure of the configuration and supporting the new pager functionality.pkg/schema/schema.go (1)
45-45: Clean schema implementation for the DisablePager field.The new
DisablePagerfield is properly tagged for all serialization formats and follows the established naming conventions of the codebase. This field allows the pager feature to be controlled via configuration files, CLI flags, and environment variables.cmd/describe.go (1)
18-18: Good CLI flag implementation.The
disable-pagerflag is properly added as a persistent flag to make it available across all describe subcommands. The default value offalseensures paging is enabled by default, which aligns with the PR objectives to improve demo readability.pkg/config/load.go (2)
104-104: Environment variable binding looks correct.The
ATMOS_DISABLE_PAGERenvironment variable is properly bound to thedisable_pagerconfiguration key, following the established pattern for environment variable configuration in the codebase.
122-122: Default configuration value is appropriate.Setting the default value to
falseensures that the pager is enabled by default, which aligns with the PR objective to improve rendering of demos through paging.pkg/utils/yaml_utils.go (1)
43-50: Good refactoring of PrintAsYAMLThe refactoring simplifies the function by delegating the detailed implementation to the new GetAtmosConfigYAML function. Excellent separation of concerns.
pkg/pager/pager.go (2)
8-11: Good use of interface for PageCreatorDefining a PageCreator interface with a single Run method makes the code more testable and facilitates mocking. The go:generate directive for mockgen is a nice touch to automate mock generation.
13-21: Effective dependency injection patternThe pageCreator struct uses dependency injection by accepting a tea program creation function. This approach makes the code more testable by allowing the injection of mock implementations.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 17-20: pkg/pager/pager.go#L17-L20
Added lines #L17 - L20 were not covered by testsinternal/exec/describe_config_test.go (3)
17-33: Good test structure and coverageThe test function covers the creation of a new describe config executor and verifies core dependencies are properly initialized.
34-50: Excellent TTY handling test caseGood test for YAML output with proper TTY support. The mock expectations are clear and the test verifies that the pager is used when TTY is available.
114-123: Good error handling testProperly verifies that malformed query expressions return appropriate errors with descriptive messages.
pkg/pager/mock_pager.go (1)
1-48: LGTM - Auto-generated mockThis mock was correctly generated by MockGen and follows standard Go mock patterns. The warnings about code coverage can be ignored as this is an auto-generated file designed for testing purposes.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-28: pkg/pager/mock_pager.go#L25-L28
Added lines #L25 - L28 were not covered by tests
[warning] 32-33: pkg/pager/mock_pager.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 37-41: pkg/pager/mock_pager.go#L37-L41
Added lines #L37 - L41 were not covered by tests
[warning] 45-47: pkg/pager/mock_pager.go#L45-L47
Added lines #L45 - L47 were not covered by testspkg/pager/model_test.go (1)
21-82: Comprehensive Update method testsGood test coverage for all relevant message types and state transitions in the Update method, including key events, window sizing, and viewport updates.
pkg/pager/model.go (3)
26-35: Well-structured pager modelThe model struct and Init method are correctly implemented, following Bubble Tea conventions with pointer receivers to satisfy the tea.Model interface.
37-75: Good handling of window resize eventsThe Update method correctly handles initialization and resize events, with proper delegation to the viewport component for keyboard and mouse events.
You're correctly using pointer receivers for the model methods, which addresses the previously flagged "hugeParam" static analysis warnings.
77-82: Clean, concise View implementationThe View method properly composes the UI with header, content, and footer while handling the uninitialized state.
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
samtholiya
force-pushed
the
feature/dev-3131-add-pager-to-atmos-describe-config-command
branch
from
e885be7 to
f557e06
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/pager/err.go (1)
5-5: DelegatedError()implementation
TheError()method properly satisfies theerrorinterface by forwarding to the wrapped error’sError(). For extra clarity, you might add a brief GoDoc comment above the method to explain its intent.pkg/utils/utils.go (2)
17-32: Consider supporting additional frontmatter formatsThe implementation correctly detects and removes YAML frontmatter, but only handles the YAML format (using
---delimiters). You might want to consider supporting other common frontmatter formats like TOML (using+++) or JSON (using{}), especially if your project supports multiple document formats.
25-26: Add a descriptive comment for the regex patternIt would be helpful to add a comment explaining what the regex pattern is matching - YAML frontmatter boundaries with optional whitespace.
-var yamlPattern = regexp.MustCompile(`(?m)^---\r?\n(\s*\r?\n)?`) +// yamlPattern matches YAML frontmatter boundaries (---) at the beginning of a line with optional whitespace +var yamlPattern = regexp.MustCompile(`(?m)^---\r?\n(\s*\r?\n)?`)
🛑 Comments failed to post (2)
pkg/utils/utils.go (2)
34-41: 🛠️ Refactor suggestion
Error handling could be improved in ExpandPath
The function silently ignores errors from
homedir.Expandand falls back to using the original path. Consider either:
- Returning the error to let callers handle it appropriately
- Logging the error before fallback
- Adding a comment explaining the fallback behavior
This would make the error handling strategy more explicit and maintainable.
72-107:
⚠️ Potential issueBug: TokyoNightStyle uses DraculaStyleConfig
There's a bug on line 104-105: TokyoNightStyle is using DraculaStyleConfig instead of its own configuration. This should be fixed.
case styles.TokyoNightStyle: - styleConfig = styles.DraculaStyleConfig + styleConfig = styles.TokyoNightStyleConfig📝 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.// GlamourStyle returns a glamour.TermRendererOption based on the given style. func GlamourStyle(style string, isCode bool) glamour.TermRendererOption { if !isCode { if style == styles.AutoStyle { return glamour.WithAutoStyle() } return glamour.WithStylePath(style) } // If we are rendering a pure code block, we need to modify the style to // remove the indentation. var styleConfig ansi.StyleConfig switch style { case styles.AutoStyle: if lipgloss.HasDarkBackground() { styleConfig = styles.DarkStyleConfig } else { styleConfig = styles.LightStyleConfig } case styles.DarkStyle: styleConfig = styles.DarkStyleConfig case styles.LightStyle: styleConfig = styles.LightStyleConfig case styles.PinkStyle: styleConfig = styles.PinkStyleConfig case styles.NoTTYStyle: styleConfig = styles.NoTTYStyleConfig case styles.DraculaStyle: styleConfig = styles.DraculaStyleConfig case styles.TokyoNightStyle: - styleConfig = styles.DraculaStyleConfig + styleConfig = styles.TokyoNightStyleConfig default: return glamour.WithStylesFromJSONFile(style) } // ...rest of function... }
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/pager/model.go (1)
256-256: 🛠️ Refactor suggestionUse pointer receiver for large struct method.
The
helpViewmethod uses a value receiver, but the model struct is large (approximately 1280 bytes). Using a pointer receiver would be more efficient as it avoids copying the entire struct.-func (m model) helpView() (s string) { +func (m *model) helpView() (s string) {🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 256-256:
hugeParam: m is heavy (1280 bytes); consider passing it by pointer
🧹 Nitpick comments (7)
pkg/utils/utils.go (3)
25-32: Consider adding a comment to explain return values.The
detectFrontmatterhelper function would benefit from a comment explaining what the returned indices represent, especially the-1, -1case.-func detectFrontmatter(c []byte) []int { +// detectFrontmatter locates YAML frontmatter boundaries in content. +// Returns [start, end] indices of the frontmatter section. +// Returns [-1, -1] when no frontmatter is detected. +func detectFrontmatter(c []byte) []int {
52-70: Document the default assumption for files without extensions.The function assumes files without extensions are markdown files. This behavior should be explicitly documented.
-// IsMarkdownFile returns whether the filename has a markdown extension. +// IsMarkdownFile returns whether the filename has a markdown extension. +// Files without extensions are assumed to be markdown files by default.
72-113: Reduce cyclomatic complexity in GlamourStyle function.The static analysis tool flagged this function as having too high cyclomatic complexity (12 > 10). Consider refactoring it to separate the style selection logic into a helper function.
func GlamourStyle(style string, isCode bool) glamour.TermRendererOption { if !isCode { if style == styles.AutoStyle { return glamour.WithAutoStyle() } return glamour.WithStylePath(style) } // If we are rendering a pure code block, we need to modify the style to // remove the indentation. + styleConfig := getStyleConfig(style) + var margin uint + styleConfig.CodeBlock.Margin = &margin + + return glamour.WithStyles(styleConfig) +} + +// getStyleConfig returns the appropriate style configuration based on the style name +func getStyleConfig(style string) ansi.StyleConfig { var styleConfig ansi.StyleConfig switch style { case styles.AutoStyle: if lipgloss.HasDarkBackground() { styleConfig = styles.DarkStyleConfig } else { styleConfig = styles.LightStyleConfig } case styles.DarkStyle: styleConfig = styles.DarkStyleConfig case styles.LightStyle: styleConfig = styles.LightStyleConfig case styles.PinkStyle: styleConfig = styles.PinkStyleConfig case styles.NoTTYStyle: styleConfig = styles.NoTTYStyleConfig case styles.DraculaStyle: styleConfig = styles.DraculaStyleConfig case styles.TokyoNightStyle: styleConfig = styles.TokyoNightStyleConfig default: - return glamour.WithStylesFromJSONFile(style) + // Return empty config for custom JSON styles + // This will be handled in the calling function + return styleConfig } - var margin uint - styleConfig.CodeBlock.Margin = &margin - - return glamour.WithStyles(styleConfig) + return styleConfigAfter this refactoring, you'll need to handle the custom JSON case specially in the
GlamourStylefunction:func GlamourStyle(style string, isCode bool) glamour.TermRendererOption { if !isCode { if style == styles.AutoStyle { return glamour.WithAutoStyle() } return glamour.WithStylePath(style) } // If we are rendering a pure code block, we need to modify the style to // remove the indentation. styleConfig := getStyleConfig(style) + // Handle custom JSON style case + if style != styles.AutoStyle && + style != styles.DarkStyle && + style != styles.LightStyle && + style != styles.PinkStyle && + style != styles.NoTTYStyle && + style != styles.DraculaStyle && + style != styles.TokyoNightStyle { + return glamour.WithStylesFromJSONFile(style) + } + var margin uint styleConfig.CodeBlock.Margin = &margin return glamour.WithStyles(styleConfig) }🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 73-73:
cyclomatic: function GlamourStyle has cyclomatic complexity 12 (> max enabled 10)pkg/pager/model.go (4)
51-51: Clean up unused style variables.The code defines styling variables that aren't referenced anywhere in the implementation. Clean up the codebase by removing these unused variables.
-var ( - lineNumberFg = lipgloss.AdaptiveColor{Light: "#656565", Dark: "#7D7D7D"} - - statusBarNoteFg = lipgloss.AdaptiveColor{Light: "#656565", Dark: "#7D7D7D"} +var ( + statusBarNoteFg = lipgloss.AdaptiveColor{Light: "#656565", Dark: "#7D7D7D"}- lineNumberStyle = lipgloss.NewStyle(). - Foreground(lineNumberFg). - Render green = lipgloss.Color("#04B575")Also applies to: 86-88
🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 51-51: var
lineNumberFgis unused(unused)
🪛 GitHub Check: golangci-lint
[failure] 51-51:
varlineNumberFgis unused
94-94: Remove unused field.The
cwdfield in thecommonModelstruct isn't used anywhere in the implementation.// Common stuff we'll need to access in all models. type commonModel struct { - cwd string width int height int }🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 94-94: field
cwdis unused(unused)
🪛 GitHub Check: golangci-lint
[failure] 94-94:
fieldcwdis unused
267-271: Create a constant for newline characters.The string literal "\n" appears multiple times. Create a named constant for better readability and maintenance.
+const ( + // existing constants + newLine = "\n" +) // In helpView method: -s += "\n" -s += "k/↑ up " + col1[0] + "\n" -s += "j/↓ down " + col1[1] + "\n" -s += "b/pgup page up " + col1[2] + "\n" -s += "f/pgdn page down " + col1[3] + "\n" -s += "u ½ page up " + col1[4] + "\n" +s += newLine +s += "k/↑ up " + col1[0] + newLine +s += "j/↓ down " + col1[1] + newLine +s += "b/pgup page up " + col1[2] + newLine +s += "f/pgdn page down " + col1[3] + newLine +s += "u ½ page up " + col1[4] + newLine🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 267-267: add-constant: string literal "\n" appears, at least, 4 times, create a named constant for it
(revive)
🪛 GitHub Check: golangci-lint
[failure] 267-267:
add-constant: string literal "\n" appears, at least, 4 times, create a named constant for it
303-367: Refactor the statusBarView method for better maintainability.This method exceeds the recommended length of 60 lines. Consider breaking it down into smaller, focused functions.
// Refactor into smaller functions: func (m *model) statusBarView(b *strings.Builder) { showStatusMessage := m.state == pagerStateStatusMessage + + scrollPercent := m.renderScrollPercent(showStatusMessage) + note := m.renderNote(showStatusMessage) + helpNote := m.renderHelpNote(showStatusMessage) + emptySpace := m.renderEmptySpace(showStatusMessage, scrollPercent, note, helpNote) + + fmt.Fprintf(b, "%s%s%s%s%s", + logo, + note, + emptySpace, + scrollPercent, + helpNote, + ) + if m.showHelp { + fmt.Fprint(b, "\n"+m.helpView()) + } } +func (m *model) renderScrollPercent(showStatusMessage bool) string { + percent := math.Max(minPercent, math.Min(maxPercent, m.viewport.ScrollPercent())) + scrollPercent := fmt.Sprintf(" %3.f%% ", percent*percentToStringMagnitude) + if showStatusMessage { + return statusBarMessageScrollPosStyle(scrollPercent) + } + return statusBarNoteStyle(scrollPercent) +}🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 303-303: function-length: maximum number of lines per function exceeded; max 60 but got 63
(revive)
🪛 GitHub Check: golangci-lint
[failure] 303-303:
function-length: maximum number of lines per function exceeded; max 60 but got 63
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
go.mod(2 hunks)pkg/pager/err.go(1 hunks)pkg/pager/model.go(1 hunks)pkg/pager/model_test.go(1 hunks)pkg/utils/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- pkg/pager/model_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/utils/utils.go (1)
pkg/config/go-homedir/homedir.go (1)
Expand(68-87)
🪛 golangci-lint (1.64.8)
pkg/pager/model.go
[error] 51-51: var lineNumberFg is unused
(unused)
[error] 86-86: var lineNumberStyle is unused
(unused)
[error] 94-94: field cwd is unused
(unused)
[error] 267-267: add-constant: string literal "\n" appears, at least, 4 times, create a named constant for it
(revive)
[error] 303-303: function-length: maximum number of lines per function exceeded; max 60 but got 63
(revive)
pkg/pager/err.go
[error] 3-3: type errMsg is unused
(unused)
[error] 5-5: func errMsg.Error is unused
(unused)
🪛 GitHub Check: golangci-lint
pkg/pager/model.go
[failure] 45-45:
Comment should end in a period
[failure] 51-51:
var lineNumberFg is unused
[failure] 86-86:
var lineNumberStyle is unused
[failure] 94-94:
field cwd is unused
[failure] 256-256:
hugeParam: m is heavy (1280 bytes); consider passing it by pointer
[failure] 267-267:
add-constant: string literal "\n" appears, at least, 4 times, create a named constant for it
[failure] 303-303:
function-length: maximum number of lines per function exceeded; max 60 but got 63
pkg/pager/err.go
[failure] 3-3:
type errMsg is unused
[failure] 5-5:
func errMsg.Error is unused
pkg/utils/utils.go
[failure] 73-73:
cyclomatic: function GlamourStyle has cyclomatic complexity 12 (> max enabled 10)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/utils/utils.go (2)
17-23: Good modular design for frontmatter removal.This function cleanly separates the responsibility of detecting frontmatter from actually removing it, making the code more maintainable.
34-41: Well-implemented path expansion.This function effectively handles both tilde expansion and environment variables, with a good fallback mechanism when tilde expansion fails.
pkg/pager/model.go (2)
113-124: Good model structure with clear separation of concerns.The model struct is well-organized with fields for content, state management, and UI components. This makes the code more maintainable and easier to understand.
137-194: Well-implemented Update method with good event handling.The Update method clearly handles different types of messages and provides appropriate responses, including keyboard navigation, viewport updates, and message passing. The workflow for each event type is logically structured.
samtholiya
force-pushed
the
feature/dev-3131-add-pager-to-atmos-describe-config-command
branch
from
68c3e83 to
7e40baf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
pkg/pager/err.go (1)
3-5: 🛠️ Refactor suggestionAdd documentation to explain the purpose of this error type.
This error message type is likely intended for use with the Bubble Tea framework's message passing system. To avoid static analysis warnings and make the code more maintainable, you should add documentation explaining its purpose.
package pager +// errMsg is an error message type designed to be used with the Bubble Tea framework. +// It wraps a standard error and is used to propagate errors through the Bubble Tea update cycle. type errMsg struct{ err error } func (e errMsg) Error() string { return e.err.Error() }If this error type is indeed not used anywhere in the codebase, consider removing it entirely to maintain clean code.
🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 3-3: type
errMsgis unused(unused)
[error] 5-5: func
errMsg.Erroris unused(unused)
🪛 GitHub Check: golangci-lint
[failure] 3-3:
typeerrMsgis unused
[failure] 5-5:
funcerrMsg.Erroris unusedpkg/pager/model.go (2)
154-157: Handle clipboard write errors.The current implementation ignores potential errors when writing to the clipboard. While it might be acceptable to continue if clipboard access fails, you should at least log the error for diagnostic purposes.
// Copy using OSC 52 termenv.Copy(StripANSI(m.content)) // Copy using native system clipboard -_ = clipboard.WriteAll(StripANSI(m.content)) +if err := clipboard.WriteAll(StripANSI(m.content)); err != nil { + cmds = append(cmds, m.showStatusMessage(pagerStatusMessage{"Failed to copy to clipboard", true})) +} else { + cmds = append(cmds, m.showStatusMessage(pagerStatusMessage{"Copied contents", false})) +} -cmds = append(cmds, m.showStatusMessage(pagerStatusMessage{"Copied contents", false}))
256-256: 🛠️ Refactor suggestionPass large struct by pointer.
The
helpViewmethod takes a large struct (1280 bytes) by value, which can be inefficient. Consider changing it to use a pointer receiver to avoid unnecessary copying.-func (m model) helpView() (s string) { +func (m *model) helpView() (s string) {🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 256-256:
hugeParam: m is heavy (1280 bytes); consider passing it by pointerpkg/utils/utils.go (2)
73-113: Refactor GlamourStyle to reduce complexity.This function has high cyclomatic complexity (12) which exceeds the maximum threshold (10). Consider refactoring to reduce complexity, perhaps by extracting the style selection into a separate function.
-// GlamourStyle returns a glamour.TermRendererOption based on the given style. -func GlamourStyle(style string, isCode bool) glamour.TermRendererOption { +// GlamourStyle returns a glamour.TermRendererOption based on the given style. +func GlamourStyle(style string, isCode bool) glamour.TermRendererOption { + if !isCode { + if style == styles.AutoStyle { + return glamour.WithAutoStyle() + } + return glamour.WithStylePath(style) + } + + // For code blocks, get the style config and customize it + styleConfig := getStyleConfig(style) + var margin uint + styleConfig.CodeBlock.Margin = &margin + + return glamour.WithStyles(styleConfig) +} + +// getStyleConfig returns the appropriate style configuration based on the style name +func getStyleConfig(style string) ansi.StyleConfig { if !isCode { if style == styles.AutoStyle { return glamour.WithAutoStyle() } return glamour.WithStylePath(style) } // If we are rendering a pure code block, we need to modify the style to // remove the indentation. var styleConfig ansi.StyleConfig switch style { case styles.AutoStyle: if lipgloss.HasDarkBackground() { styleConfig = styles.DarkStyleConfig } else { styleConfig = styles.LightStyleConfig } case styles.DarkStyle: styleConfig = styles.DarkStyleConfig case styles.LightStyle: styleConfig = styles.LightStyleConfig case styles.PinkStyle: styleConfig = styles.PinkStyleConfig case styles.NoTTYStyle: styleConfig = styles.NoTTYStyleConfig case styles.DraculaStyle: styleConfig = styles.DraculaStyleConfig case styles.TokyoNightStyle: - styleConfig = styles.DraculaStyleConfig + styleConfig = styles.TokyoNightStyleConfig default: - return glamour.WithStylesFromJSONFile(style) + return ansi.StyleConfig{} } - - var margin uint - styleConfig.CodeBlock.Margin = &margin - - return glamour.WithStyles(styleConfig) + return styleConfig }🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 73-73:
cyclomatic: function GlamourStyle has cyclomatic complexity 12 (> max enabled 10)
104-104:⚠️ Potential issueFix incorrect style configuration for TokyoNightStyle.
There appears to be a copy-paste error - TokyoNightStyle is using the Dracula style configuration.
- styleConfig = styles.DraculaStyleConfig + styleConfig = styles.TokyoNightStyleConfig
🧹 Nitpick comments (2)
pkg/pager/model.go (2)
45-45: Add a period to the comment for consistency.Every comment should end with a period for consistent code documentation style.
-// Logo +// Logo.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 45-45:
Comment should end in a period
267-271: Create a constant for recurring newline character.The string literal "\n" appears multiple times in the code. Create a named constant for better maintainability.
+const ( + // Existing constants... + newline = "\n" +) // Then throughout the code: -s += "\n" +s += newline -s += "k/↑ up " + col1[0] + "\n" +s += "k/↑ up " + col1[0] + newline // And so on for other instances🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 267-267: add-constant: string literal "\n" appears, at least, 4 times, create a named constant for it
(revive)
🪛 GitHub Check: golangci-lint
[failure] 267-267:
add-constant: string literal "\n" appears, at least, 4 times, create a named constant for it
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
go.mod(2 hunks)pkg/pager/err.go(1 hunks)pkg/pager/model.go(1 hunks)pkg/pager/model_test.go(1 hunks)pkg/utils/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- pkg/pager/model_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/utils/utils.go (1)
pkg/config/go-homedir/homedir.go (1)
Expand(68-87)
🪛 GitHub Check: golangci-lint
pkg/utils/utils.go
[failure] 73-73:
cyclomatic: function GlamourStyle has cyclomatic complexity 12 (> max enabled 10)
pkg/pager/model.go
[failure] 45-45:
Comment should end in a period
[failure] 51-51:
var lineNumberFg is unused
[failure] 86-86:
var lineNumberStyle is unused
[failure] 94-94:
field cwd is unused
[failure] 256-256:
hugeParam: m is heavy (1280 bytes); consider passing it by pointer
[failure] 267-267:
add-constant: string literal "\n" appears, at least, 4 times, create a named constant for it
[failure] 303-303:
function-length: maximum number of lines per function exceeded; max 60 but got 63
pkg/pager/err.go
[failure] 3-3:
type errMsg is unused
[failure] 5-5:
func errMsg.Error is unused
🪛 golangci-lint (1.64.8)
pkg/pager/model.go
[error] 51-51: var lineNumberFg is unused
(unused)
[error] 86-86: var lineNumberStyle is unused
(unused)
[error] 94-94: field cwd is unused
(unused)
[error] 267-267: add-constant: string literal "\n" appears, at least, 4 times, create a named constant for it
(revive)
[error] 303-303: function-length: maximum number of lines per function exceeded; max 60 but got 63
(revive)
pkg/pager/err.go
[error] 3-3: type errMsg is unused
(unused)
[error] 5-5: func errMsg.Error is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
pkg/pager/model.go (4)
370-381: Good use of helper function.The
indentfunction is a clean, well-implemented helper that efficiently handles string indentation. The comment accurately describes its purpose as a lightweight version of reflow's indent function.
126-135: Good implementation of string processing utility.The
StripANSIfunction and its regex pattern are well-implemented for removing ANSI escape sequences from text. This is essential for clipboard operations that need plain text without terminal formatting codes.
196-227: Well-designed status message system.The status message timeout implementation with proper timer management is a good design pattern. It correctly handles cleanup of previous timers and uses a command-based approach that fits well with the Bubble Tea framework's architecture.
113-125: Well-structured model design.The model struct is well-organized with clear separation of concerns for UI state, content, viewport, and status message handling. This design makes the code maintainable as features are added.
pkg/utils/utils.go (3)
34-41: The ExpandPath function handles path expansion correctly.This function properly expands both tilde (~) to the user's home directory and environment variables in paths. The error handling is appropriate, falling back to expanding only environment variables if homedir expansion fails.
17-23: The frontmatter removal implementation is clean and effective.The function properly handles YAML frontmatter in markdown files by detecting the frontmatter boundaries and removing the content between them. The implementation is straightforward and effective.
56-59:❓ Verification inconclusive
Consider revisiting the default assumption for files without extensions.
The current implementation assumes that files without extensions are markdown files. This might work for many cases but could lead to unexpected behavior for non-markdown files without extensions. Consider if this is the intended behavior for your use case.
🏁 Script executed:
#!/bin/bash # Check if there are any non-markdown files without extensions in the codebase # that might be affected by this assumption echo "Searching for files without extensions that might not be markdown..." find . -type f -not -path "*/\.*" -not -path "*/node_modules/*" -not -path "*/vendor/*" | grep -v "\." | xargs file | grep -v "text" || echo "No non-text files without extensions found."Length of output: 415
Re-evaluate handling of extension‑less files
By treating every file with no “.” in its name as Markdown, you’ll also pick up things like LICENSE, NOTICE, script files or any future binary assets without extensions. Please double‑check whether:
- You have any extension‑less resources (text or binary) that shouldn’t be rendered as Markdown
- This default behavior is truly desired, or if it should be driven by an explicit flag/whitelist
- You’d prefer to require a “.md” (or a config) rather than blanket‑defaulting to Markdown
Affected location:
• pkg/utils/utils.go lines 56–59
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
samtholiya
force-pushed
the
feature/dev-3131-add-pager-to-atmos-describe-config-command
branch
from
daf011c to
3fa3c1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/exec/describe_config.go (1)
42-67: Good error handling with fallback mechanismThe command execution flow properly handles error conditions and falls back to direct printing when the pager fails. This is a user-friendly approach that maintains functionality even when the pager can't be used.
Consider adding a bit more context to the debug log message to help with troubleshooting:
- log.Debug("Failed to use pager") + log.Debug("Failed to use pager, falling back to direct output", "error", err)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
cmd/describe.go(1 hunks)cmd/describe_config.go(2 hunks)cmd/docs.go(1 hunks)go.mod(2 hunks)internal/exec/describe_config.go(1 hunks)internal/exec/describe_config_test.go(1 hunks)pkg/config/default.go(1 hunks)pkg/config/load.go(2 hunks)pkg/pager/mock_pager.go(1 hunks)pkg/pager/model.go(1 hunks)pkg/pager/model_test.go(1 hunks)pkg/pager/pager.go(1 hunks)pkg/schema/schema.go(1 hunks)pkg/schema/schema_test.go(1 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/yaml_utils.go(1 hunks)pkg/utils/yq_utils.go(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_configuration.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden(1 hunks)tests/test-cases/demo-stacks.yaml(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- tests/snapshots/TestCLICommands_atmos_describe_configuration.stderr.golden
- tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden
- pkg/pager/mock_pager.go
🚧 Files skipped from review as they are similar to previous changes (20)
- tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden
- tests/snapshots/TestCLICommands_atmos_describe_config_imports.stderr.golden
- tests/test-cases/demo-stacks.yaml
- pkg/config/default.go
- cmd/describe.go
- tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden
- pkg/config/load.go
- cmd/docs.go
- tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden
- pkg/schema/schema_test.go
- internal/exec/describe_config_test.go
- pkg/utils/json_utils.go
- pkg/utils/yaml_utils.go
- go.mod
- cmd/describe_config.go
- pkg/pager/model_test.go
- pkg/utils/yq_utils.go
- pkg/schema/schema.go
- pkg/pager/pager.go
- pkg/pager/model.go
🧰 Additional context used
🪛 GitHub Check: Build (macos-latest, macos)
internal/exec/describe_config.go
[failure] 16-16:
other declaration of ErrInvalidFormat
🪛 GitHub Check: Build (ubuntu-latest, linux)
internal/exec/describe_config.go
[failure] 16-16:
other declaration of ErrInvalidFormat
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
internal/exec/describe_config.go (5)
24-24: Good use of a static error variableDefining
ErrTTYNotSupportedas a package-level variable with a clear, descriptive message follows Go best practices.
26-31: Well-structured dependency injection patternThe
describeConfigExecstruct and constructor function follow good dependency injection principles, making the code testable and modular.Also applies to: 33-40
69-95: Well-implemented format handling in viewConfigThe
viewConfigmethod handles different output formats appropriately and performs necessary validation before using the pager.
47-47: Good use of generic typed function for evaluationUsing the generic typed
EvaluateYqExpressionWithTypefunction provides type safety for the query evaluation.
55-65: Effective conditional pager enablementThe code correctly uses the
IsPagerEnabled()method to determine whether to use the pager, which respects user configuration preferences.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/describe_config.go (2)
55-65: Consider validating format earlier.The function takes a format parameter but only validates it deep in the
viewConfigmethod. Consider validating the format parameter earlier inExecuteDescribeConfigCmdto fail fast.func (d *describeConfigExec) ExecuteDescribeConfigCmd(query, format, output string) error { + // Validate format early for faster feedback + if format != "yaml" && format != "json" { + return DescribeConfigFormatError{format} + } + var res *schema.AtmosConfiguration var err error // ... rest of function
63-63: Consider including error details in debug log.The current debug message doesn't include the actual error, which might make troubleshooting harder. Consider including the error details in the log message.
- log.Debug("Failed to use pager") + log.Debug("Failed to use pager", "error", err)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/describe_config.go(1 hunks)internal/exec/describe_config_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/describe_config_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (6)
internal/exec/describe_config.go (6)
16-22: Good error type naming.The
DescribeConfigFormatErrortype name is clear and avoids the collision withErrInvalidFormatflagged in previous builds. This implementation properly follows Go error handling conventions.
24-24: Clear error definition for TTY support.Good practice defining a static error variable for the TTY support check. This makes error handling more explicit and easier to test.
26-40: Clean dependency injection pattern.The
describeConfigExecstruct and its constructor follow solid dependency injection principles. This design improves testability by allowing dependencies to be mocked.
43-53: Appropriate query handling.The typed query evaluation with
EvaluateYqExpressionWithTypeis a good approach that leverages Go's type system. This is cleaner than the previous implementation.
69-95: Good error handling in viewConfig method.The
viewConfigmethod has proper error handling for TTY support and content generation. The method correctly checks each potential failure point and returns appropriate errors.
91-94: Clear pager execution.The pager creation and execution is straightforward and properly handles errors. Good job isolating this functionality through the
pageCreatorinterface.
|
@samtholiya update screenshots in PR description |
|
💥 This pull request now has conflicts. Could you fix it @samtholiya? 🙏 |
what
--pagerflag,ATMOS_PAGERenv boolean variable added to enable/disable pager if requiredcto copy the contentswhy
references
Summary by CodeRabbit
New Features
settings.terminal.pagerstring setting andATMOS_PAGERenvironment variable.pagerflag to the describe commands for controlling paging behavior.Improvements
Bug Fixes
Tests
Chores