add terminal display to ease parsing for command line#93
add terminal display to ease parsing for command line#93pantelm wants to merge 1 commit intoopenhue:mainfrom
Conversation
WalkthroughThe changes introduce a Changes
Sequence DiagramsequenceDiagram
participant User
participant get_light
participant utils
participant IO
User->>get_light: RunGetLightCmd(--terminal)
rect rgb(200, 220, 255)
Note over get_light: JSON format check
get_light->>get_light: o.JSON == true?
end
alt JSON output
get_light->>IO: PrintJSON()
else Terminal output (NEW)
rect rgb(220, 255, 220)
get_light->>utils: PrintTerminal()
utils->>IO: Println() per light
end
else Table output (default)
get_light->>IO: PrintTable()
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/get/get.go(2 hunks)cmd/get/get_light.go(1 hunks)util/utils.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
util/utils.go (1)
openhue/io.go (1)
IOStreams(21-29)
cmd/get/get_light.go (1)
util/utils.go (1)
PrintTerminal(48-52)
🔇 Additional comments (3)
cmd/get/get.go (1)
11-12: LGTM!The Terminal field addition follows the same pattern as the existing Json field and integrates cleanly into the options struct.
util/utils.go (1)
48-52: LGTM!The PrintTerminal function implementation is clean and minimal, correctly achieving the PR goal of printing output without headers or blank lines. The generic signature allows reuse across different data types.
cmd/get/get_light.go (1)
67-68: Verify that tab-separated format aligns with your actual use case needs.The code review comment is factually accurate:
PrintLightdoes use tab-separated values (confirmed at lines 88-93). The--terminalflag correctly passes this function toutil.PrintTerminal, which prints each light as a tab-delimited string.However, the design question raised is valid: tab-separated values are a standard CLI format and your implementation already achieves the stated goal by removing headers and blank lines. Whether tab-separated output is the most appropriate choice depends on your actual usage patterns. If you expect users to pipe this to other tools, TSV is reasonable; if you need maximum simplicity, other formats exist (JSON, newline-separated fields, etc.).
Review the actual usage patterns to confirm tab-separated output meets your users' needs for CLI tool integration.
| cmd.PersistentFlags().BoolVarP(&o.Json, "json", "j", false, "Format output as JSON") | ||
| cmd.PersistentFlags().BoolVarP(&o.Terminal, "terminal", "c", false, "Format from Terminal use") |
There was a problem hiding this comment.
🧩 Analysis chain
Verify behavior when both --json and --terminal flags are set.
There's no mutual exclusion check between the --json and --terminal flags. Users could potentially set both flags simultaneously, which may lead to unclear behavior (the first condition wins, so JSON would be used).
Consider adding validation to ensure these flags are mutually exclusive. Run the following script to check if there are any existing patterns for flag mutual exclusion in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for mutual exclusion patterns between flags
rg -nP --type=go -C3 'MarkFlagsMutuallyExclusive|cobra\.ExactArgs|OnlyOneOf'Length of output: 1328
I need to examine the full context of cmd/get/get.go to verify whether the mutual exclusion validation is indeed missing.
</parameter>
</invoke>
---
🏁 Script executed:
```shell
#!/bin/bash
# View the full get.go file to see if mutual exclusion is set anywhere
cat cmd/get/get.go
Length of output: 1809
🏁 Script executed:
#!/bin/bash
# Search for any validation logic or usage of Json and Terminal flags in get.go
rg -nP --type=go -C5 'o\.Json|o\.Terminal' cmd/get/get.goLength of output: 806
🏁 Script executed:
#!/bin/bash
# Check if MarkFlagsMutuallyExclusive is used anywhere in get.go
rg -n 'MarkFlagsMutuallyExclusive' cmd/get/get.goLength of output: 0
Now let me verify if the o.Terminal flag is actually used in the sub-commands:
🏁 Script executed:
#!/bin/bash
# Search for Terminal usage in all get-related command files
rg -n 'o\.Terminal' cmd/get/Length of output: 162
Let me examine the usage context in get_light.go:
🏁 Script executed:
#!/bin/bash
# View the context around line 67 in get_light.go to see the flag usage logic
sed -n '55,75p' cmd/get/get_light.goLength of output: 526
Add mutual exclusion validation for --json and --terminal flags
The concern is valid. The if/else-if/else logic in cmd/get/get_light.go:60-67 confirms that when both flags are set, JSON format takes precedence and Terminal formatting is ignored. The codebase already establishes the pattern for this via MarkFlagsMutuallyExclusive in cmd/set/common.go.
Add after line 38 in cmd/get/get.go:
cmd.MarkFlagsMutuallyExclusive("json", "terminal")🤖 Prompt for AI Agents
In cmd/get/get.go around lines 37-38, add a mutual exclusion declaration for the
--json and --terminal flags by calling cmd.MarkFlagsMutuallyExclusive("json",
"terminal") immediately after those flag definitions; follow the existing
project pattern for handling the returned error (check the error and
handle/log/propagate it the same way as in cmd/set/common.go) so the CLI will
surface a clear validation error when both flags are provided.
|
|
||
| // persistence flags | ||
| cmd.PersistentFlags().BoolVarP(&o.Json, "json", "j", false, "Format output as JSON") | ||
| cmd.PersistentFlags().BoolVarP(&o.Terminal, "terminal", "c", false, "Format from Terminal use") |
There was a problem hiding this comment.
Fix grammatical error in flag help text.
The help text "Format from Terminal use" is grammatically incorrect.
Apply this diff:
- cmd.PersistentFlags().BoolVarP(&o.Terminal, "terminal", "c", false, "Format from Terminal use")
+ cmd.PersistentFlags().BoolVarP(&o.Terminal, "terminal", "c", false, "Format for terminal use")📝 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.
| cmd.PersistentFlags().BoolVarP(&o.Terminal, "terminal", "c", false, "Format from Terminal use") | |
| cmd.PersistentFlags().BoolVarP(&o.Terminal, "terminal", "c", false, "Format for terminal use") |
🤖 Prompt for AI Agents
In cmd/get/get.go around line 38, the flag help text "Format from Terminal use"
is grammatically incorrect; update the BoolVarP help string to a correct phrase
such as "Format for terminal use" or "Format output for terminal use" so the
flag description is clear and idiomatic.
|
Salut @thibauult , I'll give it a try, I'm not really familiar with jq |
2 participants
Print without header and blank line to ease parsing with cli tools
Summary by CodeRabbit
--terminal(short-c) flag to the get command, providing a terminal-friendly output format option alongside the existing JSON format for displaying light information.