feat: [#592] Add --no-ansi flag to disable color output in Artisan co…#880
feat: [#592] Add --no-ansi flag to disable color output in Artisan co…#880
Conversation
WalkthroughThe pull request enhances the CLI application's flag and color handling. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant UserCLI as User/CLI
participant App as Application.Run
participant Env as env.IsNoANSI()
participant Color as color.Disable()
UserCLI->>App: Execute CLI command with/without --no-ansi
App->>Env: Check if no-ansi flag set via environment
alt no-ansi flag is set
App->>Color: Call Disable() to disable ANSI colors
else flag not set
App-->>UserCLI: Proceed with colored output
end
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
6280320 to
e3947a5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #880 +/- ##
==========================================
+ Coverage 67.51% 67.54% +0.02%
==========================================
Files 151 151
Lines 10355 10386 +31
==========================================
+ Hits 6991 7015 +24
- Misses 2996 3001 +5
- Partials 368 370 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
console/cli_helper.go (1)
252-253: Add test coverage for newly introduced code paths.The static analysis shows untested lines in the help-printing and wrapping logic (
onUsageError,wrap,wrapLine, etc.). Encouraging thorough test coverage will help prevent regressions in printing and formatting behavior. Consider adding unit tests that invoke CLI usage and error scenarios, verifying that outputs match expectations.Also applies to: 282-284, 293-298, 334-334, 340-342, 355-359, 361-370, 373-373
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 252-252: console/cli_helper.go#L252
Added line #L252 was not covered by testscontracts/console/command/command.go (1)
27-33: New fieldDisableDefaultTextis a useful addition.This field helps hide default values in command usage displays. Ensure edge cases are tested—for example, when a flag has both a default value and
DisableDefaultTextset.support/env/env.go (2)
69-78: Add test coverage for the IsNoANSI function.The function logic is correct, but it lacks test coverage. Consider adding test cases to verify the behavior when the flag is present and absent.
Would you like me to help generate test cases for this function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 73-74: support/env/env.go#L73-L74
Added lines #L73 - L74 were not covered by tests
69-78: Consider optimizing flag checking for multiple arguments.When checking multiple command-line arguments, using a map can be more efficient than iterating through the slice multiple times in different functions.
Here's a suggested optimization that can be applied to all flag-checking functions:
+var ( + // flagCache stores preprocessed command-line flags for efficient lookup + flagCache map[string]bool +) + +func init() { + flagCache = make(map[string]bool) + for _, arg := range os.Args { + flagCache[arg] = true + } +} + func IsNoANSI() bool { - for _, arg := range os.Args { - if arg == "--no-ansi" { - return true - } - } - return false + return flagCache["--no-ansi"] }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 73-74: support/env/env.go#L73-L74
Added lines #L73 - L74 were not covered by testssupport/color/color.go (1)
199-202: Add test coverage for the DisableColor function.The function is correctly implemented but lacks test coverage. Consider adding test cases to verify that color output is properly disabled.
Would you like me to help generate test cases for this function? The test should verify that color codes are not present in the output after calling DisableColor().
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 200-201: support/color/color.go#L200-L201
Added lines #L200 - L201 were not covered by testsconsole/application.go (1)
88-90: Add test coverage for color disabling logic.The color disabling logic is correctly implemented but lacks test coverage. Consider adding test cases to verify that colors are disabled when either noANSI is true or env.IsNoANSI() returns true.
Would you like me to help generate test cases for this logic?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-90: console/application.go#L89-L90
Added lines #L89 - L90 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
console/application.go(5 hunks)console/cli_helper.go(5 hunks)console/cli_helper_test.go(1 hunks)console/console/build_command.go(0 hunks)contracts/console/command/command.go(1 hunks)database/console/migration/migrate_command.go(1 hunks)support/color/color.go(1 hunks)support/env/env.go(1 hunks)
💤 Files with no reviewable changes (1)
- console/console/build_command.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
support/color/color.go
[warning] 200-201: support/color/color.go#L200-L201
Added lines #L200 - L201 were not covered by tests
support/env/env.go
[warning] 73-74: support/env/env.go#L73-L74
Added lines #L73 - L74 were not covered by tests
console/cli_helper.go
[warning] 252-252: console/cli_helper.go#L252
Added line #L252 was not covered by tests
[warning] 282-284: console/cli_helper.go#L282-L284
Added lines #L282 - L284 were not covered by tests
[warning] 293-298: console/cli_helper.go#L293-L298
Added lines #L293 - L298 were not covered by tests
[warning] 334-334: console/cli_helper.go#L334
Added line #L334 was not covered by tests
[warning] 340-342: console/cli_helper.go#L340-L342
Added lines #L340 - L342 were not covered by tests
[warning] 355-359: console/cli_helper.go#L355-L359
Added lines #L355 - L359 were not covered by tests
[warning] 361-370: console/cli_helper.go#L361-L370
Added lines #L361 - L370 were not covered by tests
[warning] 373-373: console/cli_helper.go#L373
Added line #L373 was not covered by tests
console/application.go
[warning] 89-90: console/application.go#L89-L90
Added lines #L89 - L90 were not covered by tests
🔇 Additional comments (7)
console/cli_helper.go (3)
30-56: Good organization of help templates.Grouping the templates under a single
varblock enhances readability and consistency. The sorting of commands/flags within the templates is also a welcome improvement for user clarity.
73-80: Consider potential global state conflicts fornoANSI.Defining a package-level
noANSIvariable is typically fine for short-lived CLI processes, but be aware that if the CLI is invoked repeatedly in a long-running process (e.g., embedded contexts), it could introduce unexpected behavior.
288-290: Disable color logic looks correct.The condition
if noANSI || env.IsNoANSI()is straightforward, ensuring color is disabled if either the flag or environment variable is set. Confirm that other parts of the CLI don’t attempt to forcibly re-enable color once disabled.database/console/migration/migrate_command.go (1)
32-32: Category removal is consistent with top-level commands.The removal of
Category: "migrate"aligns with the shift away from categorization. Verify that downstream code does not rely on this field for migration grouping or help-text filtering.console/cli_helper_test.go (1)
101-114: LGTM! Comprehensive test case for --no-ansi flag.The test case thoroughly verifies the output format when ANSI colors are disabled.
console/application.go (2)
30-30: LGTM! Consistent flag integration.The noANSIFlag is properly integrated into both the application-level flags and command-level flags.
Also applies to: 49-49
134-139: LGTM! Proper flag configuration.The BoolFlag configuration is correctly updated to include the DisableDefaultText property.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
support/color/color.go (1)
199-202: Add test coverage for theDisablefunction.The function logic is correct, but it lacks test coverage. Please add test cases to verify that color output is properly disabled.
Would you like me to help generate test cases for this function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 200-201: support/color/color.go#L200-L201
Added lines #L200 - L201 were not covered by testsconsole/application.go (1)
98-100: Add test coverage for the color disabling logic.The color disabling logic is correct, but it lacks test coverage. Please add test cases to verify that color output is properly disabled when either the flag is set or the environment variable is true.
Would you like me to help generate test cases for this functionality?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 99-100: console/application.go#L99-L100
Added lines #L99 - L100 were not covered by testsconsole/cli_helper.go (2)
278-280: Add test coverage for the color disabling logic in help printer.The color disabling logic is correct, but it lacks test coverage. Please add test cases to verify that help output is properly rendered without color when either the flag is set or the environment variable is true.
Would you like me to help generate test cases for this functionality?
315-364: Add test coverage for text wrapping functions.The text wrapping functions lack test coverage. Please add test cases to verify that text is properly wrapped, especially for edge cases like empty lines and long words.
Would you like me to help generate test cases for these functions?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 324-324: console/cli_helper.go#L324
Added line #L324 was not covered by tests
[warning] 330-332: console/cli_helper.go#L330-L332
Added lines #L330 - L332 were not covered by tests
[warning] 345-349: console/cli_helper.go#L345-L349
Added lines #L345 - L349 were not covered by tests
[warning] 351-360: console/cli_helper.go#L351-L360
Added lines #L351 - L360 were not covered by tests
[warning] 363-363: console/cli_helper.go#L363
Added line #L363 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
console/application.go(5 hunks)console/cli_helper.go(5 hunks)support/color/color.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
console/application.go
[warning] 99-100: console/application.go#L99-L100
Added lines #L99 - L100 were not covered by tests
console/cli_helper.go
[warning] 242-242: console/cli_helper.go#L242
Added line #L242 was not covered by tests
[warning] 272-274: console/cli_helper.go#L272-L274
Added lines #L272 - L274 were not covered by tests
[warning] 283-288: console/cli_helper.go#L283-L288
Added lines #L283 - L288 were not covered by tests
[warning] 324-324: console/cli_helper.go#L324
Added line #L324 was not covered by tests
[warning] 330-332: console/cli_helper.go#L330-L332
Added lines #L330 - L332 were not covered by tests
[warning] 345-349: console/cli_helper.go#L345-L349
Added lines #L345 - L349 were not covered by tests
[warning] 351-360: console/cli_helper.go#L351-L360
Added lines #L351 - L360 were not covered by tests
[warning] 363-363: console/cli_helper.go#L363
Added line #L363 was not covered by tests
support/color/color.go
[warning] 200-201: support/color/color.go#L200-L201
Added lines #L200 - L201 were not covered by tests
⏰ Context from checks skipped due to timeout of 300000ms (2)
- GitHub Check: test / windows (1.23)
- GitHub Check: test / windows (1.22)
🔇 Additional comments (5)
console/application.go (3)
15-22: LGTM!The flag definition is clear and well-documented.
40-40: LGTM!The flag is correctly added to the global flags.
59-59: LGTM!The flag is correctly added to each command's flags.
console/cli_helper.go (2)
31-56: LGTM!The template organization is clean and well-structured.
297-309: LGTM!The sorting functions are well-implemented and improve the user experience by providing consistent ordering of commands and flags.
I found that the NO_COLOR env variable works natively! Just set |
Yes, it's the another way. |
I guess this might only work in a terminal or command-line environment. |
📑 Description
This PR introduces the --no-ansi flag to Artisan commands to disable color output, which is particularly useful for CI environments where colored logs can make parsing more difficult.
In addition to this feature, several other improvements have been made:
Sorted options and commands: The available options and commands are now sorted for better readability and consistency.
Command categorization: The build and migrate commands have been moved from their respective categories to the main command group, as they function more like top-level commands rather than subcommands (e.g., make:xxx).
Code formatting: The code has been formatted, and all variables and functions are now organized in alphabetical order for improved maintainability and clarity.
Closes goravel/goravel#592
Summary by CodeRabbit
✅ Checks