Add no-color support for atmos #1227
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ttps://github.com/cloudposse/atmos into feature/dev-3177--no-color-should-be-a-global-flag
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1227 +/- ##
==========================================
+ Coverage 33.56% 34.68% +1.11%
==========================================
Files 226 226
Lines 24219 24257 +38
==========================================
+ Hits 8130 8414 +284
+ Misses 14875 14605 -270
- Partials 1214 1238 +24
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:
|
…ttps://github.com/cloudposse/atmos into feature/dev-3177--no-color-should-be-a-global-flag
samtholiya
force-pushed
the
feature/dev-3177--no-color-should-be-a-global-flag
branch
from
d1297fb to
14534fb
Compare
samtholiya
force-pushed
the
feature/dev-3177--no-color-should-be-a-global-flag
branch
from
00c0e40 to
131249b
Compare
| @@ -1,35 +0,0 @@ | |||
| package cmd | |||
|
|
|||
| import ( | |||
There was a problem hiding this comment.
For some reason this test does not work in our windows workflow after our update.
I tried to check in my windows machine and this test works. Suggesting some strange issue with the worker windows version.
There was a problem hiding this comment.
I added the test to the yaml.
There was a problem hiding this comment.
Why add to yaml? That doesn’t increase or count towards test coverage and deleting it reduces test coverage.
There was a problem hiding this comment.
This is the most basic of tests, so we can just reimplement if it is wrong.
There was a problem hiding this comment.
For example, look at how you are implementing the tests for color. You are taking a different approach than what we see in this test. Maybe use the same approach.
There was a problem hiding this comment.
I already tried reimplementing it in different ways.
Just not sure why this is happening here.
The issue is outputting to the pipe buffer that we create.
But i am not sure why only this test case out of the many we have.
While i understand your perspective that this does not add coverage in yaml mode.
It covers only one line. So not sure worth the effort in debugging it for long.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis update introduces a new global Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant Logger
participant Renderer
User->>CLI: Run command with/without --no-color
CLI->>Config: Load config, parse env/flags
Config->>CLI: Provide no_color setting
CLI->>Logger: Setup logger (disable color if no_color)
CLI->>Renderer: Initialize renderer (disable color if no_color)
CLI->>User: Output (colored or plain, per config)
Possibly related PRs
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 (7)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ 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
|
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
pkg/ui/markdown/renderer.go (1)
140-147:⚠️ Potential issueUse the injected TTY helpers everywhere, not the global ones
Inside the rendering loop you fall back to
term.IsTTYSupportForStdout(), bypassing the injectedr.isTTYSupportForStdoutthat was introduced precisely to make TTY detection replaceable (e.g. for tests).
This breaks the abstraction and makes the behaviour harder to stub in unit tests.- if strings.HasPrefix(trimmed, "$") && term.IsTTYSupportForStdout() { + if strings.HasPrefix(trimmed, "$") && r.isTTYSupportForStdout() {A quick replace keeps the logic intact while preserving testability.
🧹 Nitpick comments (4)
tests/test-cases/empty-dir.yaml (1)
17-27: Add basic integration test for thesupportcommand
This new test ensures theatmos supportcommand executes successfully. Consider adding expected stdout patterns or snapshots to validate the output content in future iterations.cmd/root_test.go (1)
12-37: Good test coverage for no-color functionality.The test correctly verifies that setting the
NO_COLORenvironment variable disables color output in logs. This ensures the feature works as expected.There's a commented-out line at line 14 that appears to be duplicated by line 16 and could be removed:
- // t.Setenv("NO_COLOR", "1")pkg/config/config_test.go (1)
441-488: Comprehensive testing of no-color flag combinations.The test cases effectively verify that the
--no-colorflag is properly propagated to the configuration, including testing various combinations with log level settings. I particularly like the inclusion of the explicit false case (--no-color=false).Note that the variable name
expectetdNoColorhas a typo and should beexpectedNoColor, but it doesn't affect functionality.- expectetdNoColor bool + expectedNoColor boolpkg/ui/markdown/renderer.go (1)
80-109: Renderer re-creation on every call hurts performance
RenderWithoutWordWrapspins up a brand-new Glamour renderer every single invocation.
For large documentation generation loops this is needlessly expensive; the renderer can be prepared once (at construction time) with word-wrap0and reused, or at minimum cached behind async.Once.If immutability of
r.rendereris desired, consider adding a small cache:// inside Renderer asciiNoWrap *glamour.TermRenderer // lazily initialisedand populate it only the first time the path is taken.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 91-92: pkg/ui/markdown/renderer.go#L91-L92
Added lines #L91 - L92 were not covered by tests
[warning] 97-98: pkg/ui/markdown/renderer.go#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 107-108: pkg/ui/markdown/renderer.go#L107-L108
Added lines #L107 - L108 were not covered by tests🪛 GitHub Check: golangci-lint
[warning] 83-83:
if r.atmosConfig.Settings.Terminal.NoColorhas complex nested blocks (complexity: 4)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
cmd/root.go(3 hunks)cmd/root_test.go(1 hunks)cmd/support_test.go(0 hunks)cmd/validate_editorconfig.go(1 hunks)pkg/config/config.go(1 hunks)pkg/config/config_test.go(1 hunks)pkg/config/default.go(0 hunks)pkg/config/load.go(2 hunks)pkg/schema/schema.go(1 hunks)pkg/ui/markdown/renderer.go(4 hunks)pkg/ui/markdown/renderer_test.go(1 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Command_Line_Flag.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Config_File.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_file_in_env_should_be_priortized_over_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_file_in_flag_should_be_priortized_over_env_and_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_level_in_env_should_be_priortized_over_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_log_level_in_flag_should_be_priortized_over_env_and_config.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_about_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_generate_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_generate_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_atlantis_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(2 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden(0 hunks)tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden(0 hunks)tests/snapshots/TestCLICommands_atmos_helmfile_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_helmfile_apply_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_helmfile_apply_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_helmfile_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_support.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_--help_alias_subcommand_check.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden(1 hunks)tests/test-cases/empty-dir.yaml(1 hunks)website/docs/cli/configuration/configuration.mdx(1 hunks)website/docs/cli/configuration/terminal.mdx(2 hunks)
💤 Files with no reviewable changes (4)
- tests/snapshots/TestCLICommands_atmos_describe_configuration.stdout.golden
- tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden
- pkg/config/default.go
- cmd/support_test.go
🧰 Additional context used
🧠 Learnings (2)
pkg/config/load.go (1)
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
cmd/validate_editorconfig.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#844
File: cmd/helmfile.go:37-37
Timestamp: 2024-12-11T18:40:12.808Z
Learning: In the atmos project, `cliConfig` is initialized within the `cmd` package in `root.go` and can be used in other command files.
🧬 Code Graph Analysis (4)
pkg/config/config.go (1)
pkg/schema/schema.go (2)
Settings(716-720)Terminal(188-194)
cmd/validate_editorconfig.go (1)
pkg/schema/schema.go (1)
Terminal(188-194)
pkg/config/config_test.go (1)
pkg/schema/schema.go (2)
AtmosConfiguration(13-45)Logs(304-307)
pkg/ui/markdown/renderer.go (3)
pkg/schema/schema.go (3)
AtmosConfiguration(13-45)Settings(716-720)Terminal(188-194)internal/tui/templates/term/term_writer.go (2)
IsTTYSupportForStdout(83-87)IsTTYSupportForStderr(90-94)pkg/ui/markdown/styles.go (1)
GetDefaultStyle(24-90)
🪛 GitHub Check: codecov/patch
pkg/config/config.go
[warning] 83-84: pkg/config/config.go#L83-L84
Added lines #L83 - L84 were not covered by tests
cmd/validate_editorconfig.go
[warning] 121-121: cmd/validate_editorconfig.go#L121
Added line #L121 was not covered by tests
pkg/ui/markdown/renderer.go
[warning] 50-51: pkg/ui/markdown/renderer.go#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 91-92: pkg/ui/markdown/renderer.go#L91-L92
Added lines #L91 - L92 were not covered by tests
[warning] 97-98: pkg/ui/markdown/renderer.go#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 107-108: pkg/ui/markdown/renderer.go#L107-L108
Added lines #L107 - L108 were not covered by tests
🪛 GitHub Check: golangci-lint
pkg/ui/markdown/renderer.go
[warning] 83-83:
if r.atmosConfig.Settings.Terminal.NoColor has complex nested blocks (complexity: 4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (39)
tests/snapshots/TestCLICommands_Valid_Log_Level_in_Command_Line_Flag.stdout.golden (1)
2-2: Snapshot environment label updated
The platform identifier was changed fromdarwin/arm64tolinux/amd64to match the test environment. No functional changes here.tests/snapshots/TestCLICommands_Valid_log_level_in_env_should_be_priortized_over_config.stdout.golden (1)
2-2: Snapshot environment label updated
This updates the platform label tolinux/amd64, aligning with the environment where tests ran. No logic affected.tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stdout.golden (1)
2-2: Snapshot environment label updated
Platform identifier updated tolinux/amd64for consistency with the CI environment. No functional change.tests/snapshots/TestCLICommands_Valid_Log_Level_in_Config_File.stdout.golden (1)
2-2: Snapshot environment label updated
Changed tolinux/amd64to reflect the testing environment. Functional behavior remains unchanged.tests/snapshots/TestCLICommands_Valid_log_level_in_flag_should_be_priortized_over_env_and_config.stdout.golden (1)
2-2: Snapshot environment label updated
Updated the platform tag tolinux/amd64, matching the environment. No logic modification.tests/snapshots/TestCLICommands_Valid_log_file_in_env_should_be_priortized_over_config.stdout.golden (1)
7-7: Update snapshot platform identifier
The change from “darwin/arm64” to “linux/amd64” aligns with the current CI environment and has no impact on functionality.tests/snapshots/TestCLICommands_atmos_terraform_help.stdout.golden (1)
111-113: Add global--no-colorflag to help output
The new flag is correctly placed under Global Flags with consistent indentation and description, matching the PR’s goal of disabling colored output.tests/snapshots/TestCLICommands_atmos_terraform_apply_--help.stdout.golden (1)
49-49: Add global--no-colorflag to apply help
The flag is properly integrated into the Global Flags section and mirrors the formatting in other commands.tests/snapshots/TestCLICommands_Valid_log_file_in_flag_should_be_priortized_over_env_and_config.stdout.golden (1)
7-7: Update snapshot platform identifier
Switching to “linux/amd64” reflects the testing environment update and does not affect functionality.tests/snapshots/TestCLICommands_atmos_about_--help.stdout.golden (1)
37-37: Add global--no-colorflag to about help
Placement under Global Flags is consistent with other help outputs, ensuring uniform documentation of the new flag.tests/snapshots/TestCLICommands_atmos_helmfile_--help.stdout.golden (1)
56-56: Approved: Snapshot updated foratmos helmfile --help
The new--no-colorglobal flag is present with(default false)as expected.tests/snapshots/TestCLICommands_atmos_atlantis_--help.stdout.golden (1)
45-45: Approved: Snapshot updated foratmos atlantis --help
The--no-colorglobal flag appears correctly with default false.tests/snapshots/TestCLICommands_atmos_atlantis_help.stdout.golden (1)
45-45: Approved: Snapshot updated foratmos atlantis help
The global--no-coloroption is correctly shown with(default false).tests/snapshots/TestCLICommands_atmos_helmfile_apply_help.stdout.golden (1)
38-38: Approved: Snapshot updated foratmos helmfile apply --help
The--no-colorflag is now listed under Global Flags with default false.tests/snapshots/TestCLICommands_atmos_helmfile_help.stdout.golden (1)
56-56: Approved: Snapshot updated foratmos helmfile help
The global--no-colorflag appears as intended with(default false).tests/snapshots/TestCLICommands_atmos_helmfile_apply_--help.stdout.golden (1)
38-39: LGTM! No-color global flag is properly implemented.The
--no-colorflag has been added as a global flag with clear documentation and default value (false), addressing the accessibility needs mentioned in the PR objectives.tests/snapshots/TestCLICommands_atmos_terraform_--help.stdout.golden (1)
111-112: Global flag consistently applied to terraform command.The
--no-colorflag is consistently documented with the same description and default value as in other commands, maintaining a unified CLI interface.tests/snapshots/TestCLICommands_atmos_terraform_apply_help.stdout.golden (1)
49-50: No-color flag correctly added to terraform apply subcommand.The global
--no-colorflag is properly documented in the terraform apply help output, ensuring users know about this accessibility option when viewing subcommand help.tests/snapshots/TestCLICommands_atmos_atlantis_generate_help.stdout.golden (1)
48-49: No-color flag correctly implemented in atlantis command.The global
--no-colorflag has been consistently applied to the atlantis generate command help output. This completes the implementation across all reviewed CLI commands.tests/snapshots/TestCLICommands_atmos_atlantis_generate_--help.stdout.golden (1)
48-49: New global flag looks good.The
--no-colorflag addition is consistent with the PR objective to provide a way to disable colored output. This feature will help users with custom terminals who find colored output difficult to read.tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_--help.stdout.golden (1)
85-86: Global flag placement is consistent.The
--no-colorflag appears in the correct section under Global Flags with appropriate formatting and consistent description.tests/snapshots/TestCLICommands_atmos_atlantis_generate_repo-config_help.stdout.golden (1)
85-86: Flag description is clear and properly positioned.The
--no-colorflag has been properly added to the Global Flags section with a clear description that aligns with its purpose.tests/snapshots/TestCLICommands_atmos_terraform_--help_alias_subcommand_check.stdout.golden (1)
116-117: Global flag consistently implemented.The
--no-colorflag has been properly added to the Terraform command help output, maintaining consistency across all command help displays.tests/snapshots/TestCLICommands_atmos_validate_editorconfig_help.stdout.golden (1)
73-73: Include global--no-colorflag in help snapshot
The updated snapshot correctly reflects the addition of the global--no-coloroption with its description and default.tests/snapshots/TestCLICommands_atmos_--help.stdout.golden (1)
60-60: Update global help snapshot to include--no-color
The addition of the--no-colorflag to the global help output is correct and matches the new CLI behavior.tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden (2)
80-81: Reflect unifiedno_colorsetting in configuration snapshot
The snapshot now includes"no_color": falseundersettings.terminalas expected.
150-150: Remove expliciteditorconfig.colorproperty
Theeditorconfigsection is correctly simplified to{}, relying on the globalno_colorsetting instead of a per-command color flag.tests/snapshots/TestCLICommands_atmos_validate_editorconfig_--help.stdout.golden (1)
73-73: Looks good - supports the new no-color global flagThe addition of
--no-coloras a global flag enhances accessibility for users with custom terminals where colored output might be hard to read.tests/snapshots/TestCLICommands_atmos_describe_config_imports.stdout.golden (1)
123-123: Configuration schema correctly updated for no-colorThe addition of
no_color: falsesetting in the terminal configuration section ensures that the no-color option is properly represented in the config schema, while maintaining colored output as the default behavior.pkg/schema/schema.go (1)
193-193: Schema update looks goodThe
NoColorfield added to theTerminalstruct properly supports the new feature with appropriate struct tags for YAML, JSON, and mapstructure.cmd/root.go (2)
99-108: Well implemented color disabling for logs.The implementation properly disables colors in logs when
NoColoris set, while preserving other style attributes. This provides good accessibility for users with custom terminals who have difficulty reading colored output.
208-208: Good addition of global --no-color flag.Adding this as a persistent flag at the root command level is the right approach, as it applies globally across all commands rather than requiring command-specific implementations.
website/docs/cli/configuration/terminal.mdx (2)
30-32: Properly updated terminal settings documentation.The configuration example has been correctly updated to use
no_color: falseinstead of the previouscolorssetting, maintaining consistency with the implementation.
81-83: Clear documentation for the new no_color option.The added documentation for the
no_colorsetting is clear and concise, making it easy for users to understand how to disable colored output.pkg/config/load.go (2)
101-101: Good environment variable binding for no_color setting.Binding both
ATMOS_NO_COLORand standardNO_COLORenvironment variables follows best practices and provides users with flexibility. This correctly usesviper.BindEnvrather than directos.Getenvcalls as recommended.
118-118: Appropriate default for no_color setting.Setting the default to
falseensures colors remain enabled by default, which is the expected behavior.tests/snapshots/TestCLICommands_atmos_support.stdout.golden (1)
1-19: New snapshot file looks good.This new snapshot captures the expected plain-text output for the
atmos supportcommand, which aligns with the PR objective of adding no-color support for terminals that have difficulty with colored output.pkg/config/config_test.go (1)
407-439: Good test coverage for flag parsing.The test cases comprehensively cover parsing different flag configurations, which is important for validating the new
--no-colorflag functionality.pkg/ui/markdown/renderer_test.go (1)
1-101: Well-structured tests for color rendering.These tests comprehensively verify that the markdown renderer properly handles the no-color setting. Good practices observed:
- Testing both color and no-color scenarios
- Mocking TTY support functions for consistent testing
- Properly restoring original functions after tests
- Testing multiple rendering methods including error rendering
This ensures the no-color support works correctly throughout the rendering system.
| terminal: | ||
| no_color: false # Disable color output (Can also be set using 'ATMOS_NO_COLOR' or 'NO_COLOR' ENV var or '--no-color' command-line argument) | ||
| max_width: 120 # Maximum width for terminal output | ||
| pager: true # Use pager for long output |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document the no_color setting under settings.terminal
The default example includes no_color, but the descriptive section for settings.terminal below does not list it. Please add a documentation entry explaining the no_color option.
Proposed diff:
<dt>`pager`</dt>
<dd>Use pager for long output</dd>
+ <dt>`no_color`</dt>
+ <dd>Disable color output. Can also be set via `ATMOS_NO_COLOR` or `NO_COLOR` env var or `--no-color` flag.</dd>📝 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.
| terminal: | |
| no_color: false # Disable color output (Can also be set using 'ATMOS_NO_COLOR' or 'NO_COLOR' ENV var or '--no-color' command-line argument) | |
| max_width: 120 # Maximum width for terminal output | |
| pager: true # Use pager for long output | |
| <dt>`pager`</dt> | |
| <dd>Use pager for long output</dd> | |
| <dt>`no_color`</dt> | |
| <dd>Disable color output. Can also be set via `ATMOS_NO_COLOR` or `NO_COLOR` env var or `--no-color` flag.</dd> |
🤖 Prompt for AI Agents (early access)
In website/docs/cli/configuration/configuration.mdx around lines 121 to 124, the no_color setting under settings.terminal is missing documentation. Add a descriptive entry for no_color explaining that it disables color output in the terminal and can also be set using the ATMOS_NO_COLOR or NO_COLOR environment variables or the --no-color command-line argument.
| if flagKeyValue, ok := flagKeyValue["no-color"]; ok || flagKeyValue == "true" { | ||
| atmosConfig.Settings.Terminal.NoColor = true | ||
| } else if flagKeyValue == "false" { | ||
| atmosConfig.Settings.Terminal.NoColor = false | ||
| } |
There was a problem hiding this comment.
Implementation handles flag parsing correctly
The code correctly processes the --no-color flag, setting the terminal configuration accordingly. The implementation properly handles both presence of the flag and explicit true/false values.
However, I noticed that the code coverage tools reported these lines aren't covered by tests. Consider adding test cases for these new lines to ensure robustness:
#!/bin/bash
# Check if there are tests for the no-color flag parsing
rg -A 3 "no.?color.*test" pkg/config/🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 83-84: pkg/config/config.go#L83-L84
Added lines #L83 - L84 were not covered by tests
🤖 Prompt for AI Agents (early access)
In pkg/config/config.go around lines 80 to 84, the code correctly handles the --no-color flag but lacks test coverage. Add unit tests that explicitly cover scenarios where the flag is present with no value, set to "true", and set to "false" to ensure these branches are exercised and the terminal configuration is set as expected.
| cliConfig.NoColor = atmosConfig.Settings.Terminal.NoColor | ||
| if atmosConfig.Settings.Terminal.NoColor { | ||
| cliConfig.NoColor = atmosConfig.Settings.Terminal.NoColor | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redundant assignment in the conditional block.
Line 119 already assigns the same value, making the conditional check and reassignment in lines 120-121 unnecessary.
cliConfig.NoColor = atmosConfig.Settings.Terminal.NoColor
-if atmosConfig.Settings.Terminal.NoColor {
- cliConfig.NoColor = atmosConfig.Settings.Terminal.NoColor
-}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 121-121: cmd/validate_editorconfig.go#L121
Added line #L121 was not covered by tests
🤖 Prompt for AI Agents (early access)
In cmd/validate_editorconfig.go around lines 119 to 122, there is a redundant assignment where cliConfig.NoColor is set twice to the same value from atmosConfig.Settings.Terminal.NoColor. Remove the if statement and the reassignment inside it, keeping only the initial assignment on line 119 to eliminate unnecessary code duplication.
| if atmosConfig.Settings.Terminal.NoColor { | ||
| renderer, err := glamour.NewTermRenderer( | ||
| glamour.WithStandardStyle(styles.AsciiStyle), | ||
| glamour.WithWordWrap(int(r.width)), | ||
| glamour.WithColorProfile(r.profile), | ||
| glamour.WithEmoji(), | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| r.renderer = renderer | ||
| return r, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Branches for color vs no-color duplicate large option blocks
Both here and in RenderWithoutWordWrap you duplicate 7 identical glamour.With… options, differing only in the style passed (styles.AsciiStyle vs auto/custom). Extracting a tiny helper like:
func newTermRenderer(style glamour.TermStyle, wrap int, prof termenv.Profile) (*glamour.TermRenderer, error) {
return glamour.NewTermRenderer(
glamour.WithStandardStyle(style),
glamour.WithWordWrap(wrap),
glamour.WithColorProfile(prof),
glamour.WithEmoji(),
)
}will remove the duplication and slice the cognitive complexity flagged by the linter.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 50-51: pkg/ui/markdown/renderer.go#L50-L51
Added lines #L50 - L51 were not covered by tests
🤖 Prompt for AI Agents (early access)
In pkg/ui/markdown/renderer.go around lines 42 to 56, the code duplicates the same set of glamour.With... options for creating a TermRenderer in both color and no-color branches, differing only by the style used. To fix this, extract a helper function that accepts the style, wrap width, and color profile as parameters and returns a new TermRenderer with the common options applied. Replace the duplicated code blocks by calls to this helper, passing the appropriate style for each case, thereby reducing duplication and cognitive complexity.
|
💥 This pull request now has conflicts. Could you fix it @samtholiya? 🙏 |
what
why
references
--no-colorshould be a global flag #1224Summary by CodeRabbit
New Features
--no-colorflag to disable colored output in logs and terminal display.no_colorconfiguration option with environment variable support (ATMOS_NO_COLOR,NO_COLOR).Bug Fixes
Documentation
no_coloroption and flag usage.Tests
--no-colorflag, configuration parsing, and markdown rendering without color.Refactor
no_colorsetting for clarity and consistency.