feat: add a new filter function that returns remainders additionally#2019
Conversation
Signed-off-by: Frederic Wilhelm <frederic.wilhelm@sap.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bindings/go/configuration/generic/v1/spec/filter_test.go (1)
116-167: Add nil-input test cases to lock API contract.Please add cases for
options == nil(treated as empty options) andconfig == nil(returns error) so panic regressions are caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/configuration/generic/v1/spec/filter_test.go` around lines 116 - 167, Update TestFilterWithRemainder to include two additional table-driven cases: one where options == nil (treat as empty options) using a non-nil Config with entries and assert that filtered is nil and remainder contains all entries, and one where config == nil (pass nil as the first arg to FilterWithRemainder and options non-nil or nil) and assert that an error is returned. Reference the existing TestFilterWithRemainder table, the FilterWithRemainder function, and types Config and FilterOptions when adding these cases so the test verifies nil-options behave like empty FilterOptions and nil-config produces an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/go/configuration/generic/v1/spec/filter.go`:
- Around line 18-23: Add nil guards at the start of the exported function
FilterWithRemainder: validate that the input pointers config and options are not
nil and return a clear error (and nils for the two *Config return values) if
either is nil, to avoid dereferencing panics in lines that access config.Type,
config.Configurations, or options.ConfigTypes; also treat a nil
options.ConfigTypes as an empty slice (or let slices.Contains handle it) but do
not assume options itself is non-nil. Ensure the error message clearly names the
offending parameter and reference the function FilterWithRemainder in your
change.
---
Nitpick comments:
In `@bindings/go/configuration/generic/v1/spec/filter_test.go`:
- Around line 116-167: Update TestFilterWithRemainder to include two additional
table-driven cases: one where options == nil (treat as empty options) using a
non-nil Config with entries and assert that filtered is nil and remainder
contains all entries, and one where config == nil (pass nil as the first arg to
FilterWithRemainder and options non-nil or nil) and assert that an error is
returned. Reference the existing TestFilterWithRemainder table, the
FilterWithRemainder function, and types Config and FilterOptions when adding
these cases so the test verifies nil-options behave like empty FilterOptions and
nil-config produces an error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 568d25c4-6cd2-4973-8a20-e48b47ef56d3
📒 Files selected for processing (2)
bindings/go/configuration/generic/v1/spec/filter.gobindings/go/configuration/generic/v1/spec/filter_test.go
Signed-off-by: Frederic Wilhelm <frederic.wilhelm@sap.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
What this PR does / why we need it
Adds a new function
FilterWithRemainder()that also returns out-filtered configurations, so the caller can decide what to do with it, e.g. logging the out-filtered ones. As it basically does the same thing as the functionFilter()previously, we adjusted the function to useFilterWithRemainder()and redirect the out-filtered ones to the void (For more details check out the conversation).Which issue(s) this PR fixes
Related and required for #1998
Summary by CodeRabbit
New Features
Tests