feat: add url deduping - #7
Conversation
WalkthroughA new CLI command, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (dedupeCmd)
participant URL Normalizer
participant Output
User->>CLI (dedupeCmd): Invoke with URLs (args or stdin)
CLI (dedupeCmd)->>URL Normalizer: Normalize and generate signature for each URL
URL Normalizer-->>CLI (dedupeCmd): Return normalized signature
CLI (dedupeCmd)->>CLI (dedupeCmd): Track and filter duplicates
CLI (dedupeCmd)->>Output: Print deduplicated URLs
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:
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
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: 3
🧹 Nitpick comments (4)
internal/cli/dedupe.go (4)
52-53: Remove unreachable return statement.The
returnstatement afteros.Exit(1)is unreachable code.fmt.Println("Error: No input provided") os.Exit(1) -return
80-95: Handle edge cases in path normalization.The path normalization logic may not handle all edge cases correctly, particularly with empty paths or paths with only slashes.
Consider improving the path normalization logic:
-pathParts := strings.Split(strings.Trim(u.Path, "/"), "/") -var normalizedParts []string - -for _, part := range pathParts { - if part == "" { - continue - } - - if isNumeric(part) || strings.Contains(part, ".") { - normalizedParts = append(normalizedParts, "{param}") - } else { - normalizedParts = append(normalizedParts, part) - } -} - -normalizedPath := "/" + strings.Join(normalizedParts, "/") +var normalizedPath string +if u.Path == "" || u.Path == "/" { + normalizedPath = "/" +} else { + pathParts := strings.Split(strings.Trim(u.Path, "/"), "/") + var normalizedParts []string + + for _, part := range pathParts { + if part == "" { + continue + } + + if isNumeric(part) || strings.Contains(part, ".") { + normalizedParts = append(normalizedParts, "{param}") + } else { + normalizedParts = append(normalizedParts, part) + } + } + + normalizedPath = "/" + strings.Join(normalizedParts, "/") +}
111-114: Improve signature construction for better readability.The signature construction could be made more readable and maintainable.
-signature := base + normalizedPath + "|" -for _, key := range paramKeys { - signature += key + "," -} +var sigBuilder strings.Builder +sigBuilder.WriteString(base) +sigBuilder.WriteString(normalizedPath) +sigBuilder.WriteString("|") +sigBuilder.WriteString(strings.Join(paramKeys, ",")) +signature := sigBuilder.String()
127-134: Consider using strconv.Atoi for numeric validation.The manual character-by-character check could be replaced with Go's standard library function.
+import ( + "strconv" + // ... other imports +) func isNumeric(s string) bool { - for _, char := range s { - if char < '0' || char > '9' { - return false - } - } - return len(s) > 0 + _, err := strconv.Atoi(s) + return err == nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)internal/cli/dedupe.go(1 hunks)internal/cli/encode.go(0 hunks)test.sh(0 hunks)
💤 Files with no reviewable changes (2)
- internal/cli/encode.go
- test.sh
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/cli/dedupe.go (1)
internal/utils/url.go (1)
CreateURL(7-16)
🔇 Additional comments (1)
README.md (1)
80-86: Documentation looks good and matches implementation.The new documentation section clearly demonstrates the
dedupecommand usage with both direct arguments and piped input examples.
| if value == "" { | ||
| continue | ||
| } | ||
| url := utils.CreateURL(value) |
There was a problem hiding this comment.
Add error handling for invalid URLs.
The utils.CreateURL() function panics on invalid URLs, but there's no error handling in the dedupe command. This could cause the entire program to crash on invalid input.
Based on the relevant code snippet, utils.CreateURL() panics for invalid URLs. Consider wrapping these calls with error handling:
-url := utils.CreateURL(value)
+url, err := url.Parse(value)
+if err != nil {
+ fmt.Fprintf(os.Stderr, "Warning: Skipping invalid URL: %s (%v)\n", value, err)
+ continue
+}
+if url.Scheme == "" && url.Host == "" {
+ fmt.Fprintf(os.Stderr, "Warning: Skipping invalid URL: %s\n", value)
+ continue
+}Or alternatively, modify utils.CreateURL() to return an error instead of panicking.
Also applies to: 41-41
🤖 Prompt for AI Agents
In internal/cli/dedupe.go at lines 27 and 41, the calls to utils.CreateURL() can
panic on invalid URLs because there is no error handling. To fix this, wrap
these calls in a way that recovers from panics or check the validity of the URL
before calling CreateURL. Alternatively, modify utils.CreateURL() to return an
error instead of panicking and update the calling code to handle that error
gracefully, preventing the program from crashing on invalid input.
| for i := 0; i < len(paramKeys); i++ { | ||
| for j := i + 1; j < len(paramKeys); j++ { | ||
| if paramKeys[i] > paramKeys[j] { | ||
| paramKeys[i], paramKeys[j] = paramKeys[j], paramKeys[i] | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace manual sorting with Go's sort package.
The manual bubble sort implementation is inefficient and unnecessary when Go's sort package is available.
+import (
+ "sort"
+ // ... other imports
+)
-for i := 0; i < len(paramKeys); i++ {
- for j := i + 1; j < len(paramKeys); j++ {
- if paramKeys[i] > paramKeys[j] {
- paramKeys[i], paramKeys[j] = paramKeys[j], paramKeys[i]
- }
- }
-}
+sort.Strings(paramKeys)📝 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.
| for i := 0; i < len(paramKeys); i++ { | |
| for j := i + 1; j < len(paramKeys); j++ { | |
| if paramKeys[i] > paramKeys[j] { | |
| paramKeys[i], paramKeys[j] = paramKeys[j], paramKeys[i] | |
| } | |
| } | |
| } | |
| // --- at the top of internal/cli/dedupe.go, merge into your existing import block: | |
| import ( | |
| "sort" | |
| // ... other existing imports | |
| ) | |
| // ... later in the same function, replace the manual loops: | |
| - for i := 0; i < len(paramKeys); i++ { | |
| - for j := i + 1; j < len(paramKeys); j++ { | |
| - if paramKeys[i] > paramKeys[j] { | |
| - paramKeys[i], paramKeys[j] = paramKeys[j], paramKeys[i] | |
| - } | |
| - } | |
| - } | |
| + sort.Strings(paramKeys) |
🤖 Prompt for AI Agents
In internal/cli/dedupe.go around lines 103 to 109, replace the manual bubble
sort loop sorting paramKeys with a call to Go's built-in sort package. Import
"sort" if not already imported, and use sort.Strings(paramKeys) to efficiently
sort the slice instead of the nested loops.
| if len(args) > 0 { | ||
| for _, value := range args { | ||
| if value == "" { | ||
| continue | ||
| } | ||
| url := utils.CreateURL(value) | ||
| urlStr := url.String() | ||
| if !seen[urlStr] { | ||
| seen[urlStr] = true | ||
| urls = append(urls, url) | ||
| } | ||
| } | ||
| } else { | ||
| scanner := bufio.NewScanner(os.Stdin) | ||
| for scanner.Scan() { | ||
| value := scanner.Text() | ||
| if value == "" { | ||
| continue | ||
| } | ||
| url := utils.CreateURL(value) | ||
| urlStr := url.String() | ||
| if !seen[urlStr] { | ||
| seen[urlStr] = true | ||
| urls = append(urls, url) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant deduplication logic in the command handler.
The command handler implements its own deduplication using url.String() as the key, but then calls dedupeUrls() which performs a different, more sophisticated signature-based deduplication. This creates confusion and redundancy.
Remove the initial deduplication logic and let dedupeUrls() handle all deduplication:
var urls []*url.URL
-seen := make(map[string]bool)
if len(args) > 0 {
for _, value := range args {
if value == "" {
continue
}
url := utils.CreateURL(value)
- urlStr := url.String()
- if !seen[urlStr] {
- seen[urlStr] = true
- urls = append(urls, url)
- }
+ urls = append(urls, url)
}
} else {
scanner := bufio.NewScanner(os.Stdin)
for scanner.Scan() {
value := scanner.Text()
if value == "" {
continue
}
url := utils.CreateURL(value)
- urlStr := url.String()
- if !seen[urlStr] {
- seen[urlStr] = true
- urls = append(urls, url)
- }
+ urls = append(urls, url)
}
}🤖 Prompt for AI Agents
In internal/cli/dedupe.go between lines 22 and 48, remove the initial
deduplication logic that uses a map with url.String() as the key inside the
command handler. Instead, keep the code that collects all URLs from args or
stdin without filtering duplicates, and rely solely on the dedupeUrls() function
later to perform the deduplication based on its signature method. This
eliminates redundant and conflicting deduplication steps.
Docstrings generation was requested by @omarkurt. * #7 (comment) The following files were modified: * `internal/cli/dedupe.go`
|
Note Generated docstrings for this pull request at #8 |
2 participants
Summary by CodeRabbit