🐛 Protect against duplicate insights. #161
Conversation
WalkthroughA new method was added to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Insights
participant Output
User->>Insights: Write()
Insights->>Insights: ensureUnique()
Insights->>Output: Encode and write insights
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Jeff Ortel <[email protected]>
jortel
force-pushed
the
deduplicate-insights
branch
from
71b6992 to
ca1faf0
Compare
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
![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)
builder/insight.go (1)
169-191: Algorithm correctly handles duplicates but has misleading variable naming.The logic correctly detects duplicate rule IDs across violations and insights within the same ruleset and consistently modifies insights (the second collection processed) by appending an underscore suffix.
However, the variable name
violationsis misleading since it processes both violations and insights collections.Apply this diff to improve variable naming:
- for _, violations := range collections { - for ruleid, v := range violations { + for _, collection := range collections { + for ruleid, v := range collection { key := ruleset.Name + ruleid if _, found := rules[key]; found { - delete(violations, ruleid) + delete(collection, ruleid) ruleid += "_" - violations[ruleid] = v + collection[ruleid] = v } rules[key]++ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefile(1 hunks)builder/builder_test.go(1 hunks)builder/insight.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
builder/builder_test.go (1)
builder/insight.go (1)
Insights(30-34)
🪛 GitHub Actions: tackle2-addon-analyzer CI
builder/builder_test.go
[error] 28-28: go vet error: 'b.cleanInput' undefined (type Insights has no field or method cleanInput)
🔇 Additional comments (4)
Makefile (2)
8-9: LGTM! Proper inclusion of builder package tests.The addition of
./builder/...to the PKG variable correctly extends test coverage to include the new tests in the builder package.
35-35: LGTM! Test target properly updated.The test command now includes
./builder/...which ensures the newTestNextIdtest will be executed as part of the test suite.builder/insight.go (1)
46-46: LGTM! Proper integration of uniqueness enforcement.The call to
ensureUnique()before processing output ensures duplicate rule IDs are resolved before writing insights.builder/builder_test.go (1)
10-45: Test logic is correct and comprehensive.The test properly validates the duplicate rule ID resolution by:
- Setting up a scenario with overlapping rule IDs between violations and insights
- Verifying that insights get renamed with underscore suffix while violations remain unchanged
- Using appropriate test data that matches the expected behavior
Once the method name is fixed, this test will properly validate the
ensureUnique()functionality.
Signed-off-by: Jeff Ortel <[email protected]>
The same ruleset.ruleid can report both a violation and an insight. The addon needs to tolerate this: detect the duplicate and add a
_suffix to the rule id making it unique. A trailing_is minimally invasive and syntax friendly within the filter syntax.fixes #163
Summary by CodeRabbit