add GetBanner function for enhanced CLI output and double decode/encode func #3
Conversation
WalkthroughThe changes update the Go module dependencies in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (rootCmd)
participant GetBanner
participant color
User->>CLI (rootCmd): Run CLI command
CLI (rootCmd)->>GetBanner: Call GetBanner()
GetBanner->>color: Print colored ASCII banner
GetBanner-->>CLI (rootCmd): Return (prints banner as side effect)
CLI (rootCmd)->>User: Display banner and long description
sequenceDiagram
participant User
participant CLI (encodeCmd)
participant encodeURL
User->>CLI (encodeCmd): Run encode with input and flags (component, double)
CLI (encodeCmd)->>encodeURL: Call encodeURL(input, component, double)
encodeURL-->>CLI (encodeCmd): Return encoded output
CLI (encodeCmd)->>User: Display encoded output
sequenceDiagram
participant User
participant CLI (decodeCmd)
participant decodeURL
User->>CLI (decodeCmd): Run decode with input and flags (component, double)
CLI (decodeCmd)->>decodeURL: Call decodeURL(input, component, double)
decodeURL-->>CLI (decodeCmd): Return decoded output or error
CLI (decodeCmd)->>User: Display decoded output or error message
Poem
✨ 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:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/cli/root.go (1)
14-15: Consider the impact of banner placement in Long description.Using
GetBanner()here works but has side effects - the banner will be printed to stdout whenever the Long description is accessed, not just when displaying help.Consider moving the banner display to a more controlled location, such as the
Execute()function or a pre-run hook:var rootCmd = &cobra.Command{ Use: "funURL", Short: "A Swiss Army Knife for URLs", - Long: GetBanner() + ` + Long: ` funURL is a command-line tool designed as a Swiss Army knife for URL. It takes a URL as input (via stdin, command-line flags, or arguments) and provides a range of operations for parsing, validation, modification, encoding/decoding, and handling URLs efficiently.`, + PersistentPreRun: func(cmd *cobra.Command, args []string) { + PrintBanner() + }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(1 hunks)internal/cli/root.go(1 hunks)
🔇 Additional comments (3)
go.mod (2)
5-8: LGTM: Proper dependency management for colorized output.The addition of
github.com/fatih/colorin a structured require block looks good. This is a popular package for adding terminal colors in Go applications.
12-15:Details
✅ Verification successful
Verify indirect dependencies are compatible.
These indirect dependencies were automatically added to support the color package. Make sure they don't cause compatibility issues with your minimum Go version requirement.
🏁 Script executed:
#!/bin/bash # Check if these indirect dependencies have any compatibility issues go mod verify echo "Go version compatibility check:" go versionLength of output: 154
🏁 Script executed:
#!/bin/bash # Extract minimum Go version declared in go.mod grep '^go ' go.modLength of output: 30
All indirect dependencies verified successfully
go mod verifycompleted without errors- Project’s minimum Go version (
go 1.24.1) matches the current Go toolchain- No compatibility issues detected with the listed indirect dependencies
internal/cli/root.go (1)
3-8: LGTM: Clean import organization.The imports are well-organized with standard libraries first, followed by third-party packages.
| func GetBanner() string { | ||
| green := color.New(color.FgGreen).SprintFunc() | ||
| reset := color.New(color.Reset).SprintFunc() | ||
| red := color.New(color.FgRed).SprintFunc() | ||
| fmt.Println(reset(` | ||
| __ _ _ ___ _ | ||
| / _|_ _ _ _ | | | | _ \ | | ||
| | _| || | ' \| |__| | / |__ | ||
| |_| \_,_|_||_|____|_|_|_\____|`)) | ||
| fmt.Printf(" %s <3 %s \n", green("A Swiss Army Knife for URLs"), red("@HappyHackingSpace")) | ||
| fmt.Println(reset("")) | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve function naming and consider environment detection.
The function name GetBanner() suggests it returns the banner as a string, but it actually prints to stdout and returns an empty string.
Consider these improvements:
- Rename the function to
PrintBanner()to match its behavior - Add terminal detection to avoid printing in non-interactive environments
-func GetBanner() string {
+func PrintBanner() {
green := color.New(color.FgGreen).SprintFunc()
reset := color.New(color.Reset).SprintFunc()
red := color.New(color.FgRed).SprintFunc()
+
+ // Skip banner in non-interactive environments
+ if fileInfo, _ := os.Stdout.Stat(); (fileInfo.Mode() & os.ModeCharDevice) == 0 {
+ return
+ }
+
fmt.Println(reset(`
__ _ _ ___ _
/ _|_ _ _ _ | | | | _ \ |
| _| || | ' \| |__| | / |__
|_| \_,_|_||_|____|_|_|_\____|`))
fmt.Printf(" %s <3 %s \n", green("A Swiss Army Knife for URLs"), red("@HappyHackingSpace"))
fmt.Println(reset(""))
- return ""
}Committable suggestion skipped: line range outside the PR's diff.
omarkurt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/cli/encode.go (1)
19-19: Consider error handling for flag retrieval.The error from
GetBoolis being discarded with the blank identifier. Although this follows the pattern of existing code, it's generally a good practice to check for errors when retrieving flag values, especially for flags that significantly affect program behavior.- doubled, _ := cmd.Flags().GetBool("double") + doubled, err := cmd.Flags().GetBool("double") + if err != nil { + fmt.Fprintf(os.Stderr, "Error retrieving double flag: %v\n", err) + os.Exit(1) + }internal/cli/decode.go (2)
19-19: Consider error handling for flag retrieval.Similar to the encode.go file, the error from
GetBoolis being discarded. It would be more robust to check for errors when retrieving flag values.- doubled, _ := cmd.Flags().GetBool("double") + doubled, err := cmd.Flags().GetBool("double") + if err != nil { + fmt.Fprintf(os.Stderr, "Error retrieving double flag: %v\n", err) + os.Exit(1) + }
43-73: Consider adding documentation for expected input format.When using double decoding, users might not always know if their input is double-encoded. Consider adding documentation (either in the code comments or CLI help text) to clarify when double decoding should be used and what types of input it expects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cli/decode.go(2 hunks)internal/cli/encode.go(2 hunks)
🔇 Additional comments (7)
internal/cli/encode.go (3)
41-41: LGTM! Good addition of the double encoding flag.The flag is properly defined with a clear description and follows the naming pattern of the existing flags.
44-44: Function signature update is correct.The function signature has been properly updated to accept the new parameter.
49-50: Double encoding implementation looks good.The logic for double encoding is correctly implemented, using
url.QueryEscapetwice. This aligns with the description and purpose of the feature.internal/cli/decode.go (4)
40-40: Good addition of the double decoding flag.The flag is properly defined with a clear description and follows the existing pattern.
43-43: Function signature update is correct.The function signature has been properly updated to accept the new parameter.
46-58: Good implementation of double decoding with proper error handling.The double decoding implementation is well-structured with:
- Clear comments indicating the first and second pass
- Specific error messages for each decoding stage
- Proper error handling that exits on failure
This makes debugging easier if decoding fails at either stage.
59-64: Improved error messages for better debugging.The error messages are now more specific about what kind of decoding failed (query component or path), which will make debugging easier. This is a good improvement over generic error messages.
Also applies to: 67-70
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
README.md (1)
15-17:⚠️ Potential issueUpdate installation instructions to reflect removal of
install.sh
Theinstall.shscript was removed in this PR, but the README still references it. Please replace it with the new Makefile workflow (e.g.,make build) or add aninstalltarget as suggested above.-./install.sh +make build +# then optionally: sudo cp funURL /usr/local/bin
🧹 Nitpick comments (4)
Makefile (4)
1-5: Consider adding an explicitinstalltarget and clarifying install paths
You defineINSTALL_DIR(andUSER_BIN) but don’t actually provide aninstalltarget. Adding one will streamline adoption, and you can document that writing to/usr/local/bintypically requires elevated privileges (or suggest using$(USER_BIN)for user‐local installs).You could add something like:
+install: + @echo "Installing $(BINARY_NAME) to $(INSTALL_DIR)..." + @mkdir -p $(INSTALL_DIR) + @cp $(BINARY_NAME) $(INSTALL_DIR) + @echo "Installed $(BINARY_NAME) to $(INSTALL_DIR)"
10-19: Useprintffor consistent ANSI escape handling
Relying on plainechomay not interpret\033escapes portably across shells. Switching toprintf(which reliably handles\nand escape sequences) will produce consistent colored output.- @echo "\033[0;32m" + @printf "\033[0;32m\n" ... - @echo "\033[0m" + @printf "\033[0m\n"
20-32: Simplify OS detection usinggo envand widen Windows support
uname -smay reportMINGW64_NT-...orCYGWIN_NT-...on Windows, so the current check could miss those. You can useGOOS := $(shell go env GOOS)for reliable detection (windows,linux,darwin) and then branch accordingly. This also makes cross-compilation logic clearer.
33-41: Clean up all generated artifacts in one step
Right now you remove only the OS-specific binary. If you ever cross-compile or switch targets, remnants may remain. You can simplify to a single command that removes both names:-ifeq ($(OS), Windows_NT) - @rm -f $(BINARY_NAME).exe -else - @rm -f $(BINARY_NAME) -endif + @rm -f $(BINARY_NAME) $(BINARY_NAME).exe
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
img/hhs.svgis excluded by!**/*.svgimg/image.pngis excluded by!**/*.png
📒 Files selected for processing (3)
Makefile(1 hunks)README.md(2 hunks)install.sh(0 hunks)
💤 Files with no reviewable changes (1)
- install.sh
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
5-5: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
🔇 Additional comments (3)
README.md (3)
1-2: Top badge image and alt text look good
The “Made with love” badge includes meaningful alt text for accessibility. Nice!
8-10: Requirements section is clear and concise
Specifying “Golang 1.18+” aligns with the module’s Go version. No changes needed here.
124-125: Closing image alt text is present and correct
The bottom image includes the same meaningful alt text as the top badge. All good!
| .PHONY: all build clean | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add banner to .PHONY so it always runs
The banner target isn’t declared phony, so if a file named banner exists, Make will skip it. Including it in .PHONY ensures the ASCII art always prints.
-.PHONY: all build clean
+.PHONY: all banner build clean📝 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.
| .PHONY: all build clean | |
| .PHONY: all banner build clean |
|
|
||
| # funURL | ||
|
|
||
| <img src="img/image.png"> <br> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add descriptive alt text for the inline illustration
The <img src="img/image.png"> lacks alt text, which is important for accessibility and linting (MD045).
-<img src="img/image.png"> <br>
+<img src="img/image.png" alt="funURL usage illustration"> <br>📝 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.
| <img src="img/image.png"> <br> | |
| <img src="img/image.png" alt="funURL usage illustration"> <br> |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
5-5: Images should have alternate text (alt text)
null
(MD045, no-alt-text)
|
great work! looks nice |
2 participants
Summary by CodeRabbit
New Features
Chores