Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughparseFiltersValue now accepts pointer-to-named-string fields (e.g., Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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)
api/v3/filters/parse_test.go (1)
307-320: Tiny coverage add: lock down the empty-value branch.The new helper intentionally leaves the typed pointer unset for
filter[tx_type]=; adding that subtest would make the behavior explicit.🧪 Proposed test addition
func TestParse_NamedStringType(t *testing.T) { t.Run("named string type filter value", func(t *testing.T) { var f testFilter require.NoError(t, Parse(url.Values{"filter[tx_type]": {"funded"}}, &f)) require.NotNil(t, f.TxType) assert.Equal(t, testStringType("funded"), *f.TxType) }) + t.Run("nil when filter value is empty", func(t *testing.T) { + var f testFilter + require.NoError(t, Parse(url.Values{"filter[tx_type]": {""}}, &f)) + assert.Nil(t, f.TxType) + }) + t.Run("nil when no filter key present", func(t *testing.T) { var f testFilter require.NoError(t, Parse(url.Values{}, &f)) assert.Nil(t, f.TxType) }) }As per coding guidelines,
**/*_test.go: “Make sure the tests are comprehensive and cover the changes.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/filters/parse_test.go` around lines 307 - 320, Add a subtest inside TestParse_NamedStringType to cover the empty-value branch: call Parse with url.Values{"filter[tx_type]": {""}} against a fresh testFilter, require no error, and assert that f.TxType is nil (similar to the existing "nil when no filter key present" case). This makes the behavior for Parse, testFilter, TxType and the named type testStringType explicit when the filter key is present but has an empty value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/filters/parse.go`:
- Around line 168-176: The branch that handles named string pointer fields (the
block that calls parseStringPtrTyped when fieldVal.Kind()==reflect.Pointer &&
fieldVal.Type().Elem().Kind()==reflect.String) must not silently accept
operator-style keys like filter[tx_type][eq]; detect if the query string map
contains any keys of the form name+"[...]" (i.e., keys that start with the field
name plus '[') and if so return a clear error (e.g., fmt.Errorf("filter[%s]:
unsupported operator-style key", name)) instead of calling parseStringPtrTyped,
and apply the same operator-rejection logic to the analogous block later (the
similar handling around lines 202-221) so operator-style filters for named
string pointers are rejected consistently.
---
Nitpick comments:
In `@api/v3/filters/parse_test.go`:
- Around line 307-320: Add a subtest inside TestParse_NamedStringType to cover
the empty-value branch: call Parse with url.Values{"filter[tx_type]": {""}}
against a fresh testFilter, require no error, and assert that f.TxType is nil
(similar to the existing "nil when no filter key present" case). This makes the
behavior for Parse, testFilter, TxType and the named type testStringType
explicit when the filter key is present but has an empty value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 93dd77df-eb1c-4350-90bd-3596f4c04434
📒 Files selected for processing (2)
api/v3/filters/parse.goapi/v3/filters/parse_test.go
Overview
Adds support for parsing filter strings into custom string types
Notes for reviewer
Summary by CodeRabbit
New Features
Tests