fix: keyterm and keywords support for multiple values #614
Conversation
WalkthroughType signatures for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/deepgram/listen/v2/client.py (2)
157-157: Critical: Update async method signature to match sync method.The sync method at line 46 was updated to accept
Union[str, Sequence[str]], but this async method still declaresOptional[str]. This creates an inconsistency between sync and async APIs.Apply this diff to fix the inconsistency:
- keyterm: typing.Optional[str] = None, + keyterm: typing.Optional[typing.Union[str, typing.Sequence[str]]] = None,
70-70: Update docstrings to reflect the new parameter type.The docstrings at lines 70 and 181 still indicate
typing.Optional[str], but the parameter now acceptstyping.Optional[typing.Union[str, typing.Sequence[str]]]. Update both docstrings to reflect the new type.Apply these diffs:
Line 70:
- keyterm : typing.Optional[str] + keyterm : typing.Optional[typing.Union[str, typing.Sequence[str]]]Line 181:
- keyterm : typing.Optional[str] + keyterm : typing.Optional[typing.Union[str, typing.Sequence[str]]]Also applies to: 181-181
src/deepgram/listen/v1/client.py (2)
267-268: Critical: Update async method signature to match sync method.The sync method at lines 56-57 was updated to accept
Union[str, Sequence[str]]for both parameters, but this async method still declaresOptional[str]. This creates an inconsistency between sync and async APIs.Apply this diff to fix the inconsistency:
- keyterm: typing.Optional[str] = None, - keywords: typing.Optional[str] = None, + keyterm: typing.Optional[typing.Union[str, typing.Sequence[str]]] = None, + keywords: typing.Optional[typing.Union[str, typing.Sequence[str]]] = None,
100-100: Update docstrings to reflect the new parameter types.The docstrings at lines 100, 102, 311, and 313 still indicate
typing.Optional[str], but these parameters now accepttyping.Optional[typing.Union[str, typing.Sequence[str]]]. Update all four docstrings to reflect the new types.Apply these diffs:
Line 100:
- keyterm : typing.Optional[str] + keyterm : typing.Optional[typing.Union[str, typing.Sequence[str]]]Line 102:
- keywords : typing.Optional[str] + keywords : typing.Optional[typing.Union[str, typing.Sequence[str]]]Line 311:
- keyterm : typing.Optional[str] + keyterm : typing.Optional[typing.Union[str, typing.Sequence[str]]]Line 313:
- keywords : typing.Optional[str] + keywords : typing.Optional[typing.Union[str, typing.Sequence[str]]]Also applies to: 102-102, 311-311, 313-313
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/deepgram/listen/v1/client.py(1 hunks)src/deepgram/listen/v2/client.py(1 hunks)
🔇 Additional comments (1)
src/deepgram/listen/v1/client.py (1)
56-57: LGTM! Verify httpx behavior as noted in v2 client review.The type signature updates for both
keytermandkeywordsenable multi-value support. Ensure thathttpx.QueryParams.add()at lines 169-172 correctly handles sequences (same verification as requested for the v2 client).
| eot_threshold: typing.Optional[str] = None, | ||
| eot_timeout_ms: typing.Optional[str] = None, | ||
| keyterm: typing.Optional[str] = None, | ||
| keyterm: typing.Optional[typing.Union[str, typing.Sequence[str]]] = None, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify httpx.QueryParams.add() handles sequences correctly.
The type signature update looks good and enables multi-value support. However, ensure that httpx.QueryParams.add() at lines 102-103 correctly serializes a sequence into multiple query parameters with the same key (e.g., ?keyterm=val1&keyterm=val2).
Run the following script to verify httpx behavior:
🏁 Script executed:
#!/bin/bash
# Description: Test httpx.QueryParams.add() behavior with sequences
python << 'EOF'
import httpx
# Test with string
qp = httpx.QueryParams()
qp = qp.add("keyterm", "single")
print("Single string:", str(qp))
# Test with list
qp2 = httpx.QueryParams()
qp2 = qp2.add("keyterm", ["val1", "val2"])
print("List of strings:", str(qp2))
# Test with tuple
qp3 = httpx.QueryParams()
qp3 = qp3.add("keyterm", ("val1", "val2"))
print("Tuple of strings:", str(qp3))
EOFLength of output: 142
🏁 Script executed:
#!/bin/bash
# Check the actual implementation at lines 102-103 and surrounding context
sed -n '95,110p' src/deepgram/listen/v2/client.pyLength of output: 870
🏁 Script executed:
#!/bin/bash
# Check if other parameters use sequences and how they're handled
rg -n "typing.Sequence" src/deepgram/listen/v2/client.py | head -20Length of output: 84
---
🌐 Web query:
```web
httpx QueryParams add multiple values same key
💡 Result:
Use a dict whose value is a list (httpx does not accept a list-of-tuples for params). Example:
-
Simple GET with repeated key:
import httpx
r = httpx.get("https://example.com/search", params={"tag": ["red", "blue"]})-> https://example.com/search?tag=red&tag=blue
-
Using QueryParams explicitly:
from httpx import QueryParams
params = QueryParams({"tag": ["red", "blue"]})
r = httpx.get("https://example.com/search", params=params)
If you need advanced merging/serialization (custom list formats, preserving order, etc.) consider httpx-qs. [1][2]
Sources:
[1] HTTPX compatibility / Query Parameters (requests -> httpx differences). [3search2]
[2] httpx-qs (extra query-string control). [0search6]
Fix httpx.QueryParams.add() to properly handle keyterm sequences.
The type signature at line 46 allows Union[str, Sequence[str]], but the implementation at line 102 uses .add("keyterm", keyterm) which incorrectly stringifies sequences instead of expanding them into multiple query parameters.
When keyterm=["val1", "val2"] is passed, httpx produces keyterm=%5B%27val1%27%2C+%27val2%27%5D instead of the correct keyterm=val1&keyterm=val2.
Solution: Check if keyterm is a sequence and add multiple parameters, or iterate through the sequence:
if keyterm is not None:
if isinstance(keyterm, str):
query_params = query_params.add("keyterm", keyterm)
else:
for term in keyterm:
query_params = query_params.add("keyterm", term)🤖 Prompt for AI Agents
In src/deepgram/listen/v2/client.py around line 46 and the query-building at
~line 102, the code currently does query_params = query_params.add("keyterm",
keyterm) which stringifies sequences; change it to detect if keyterm is None, a
str, or a sequence (e.g., isinstance(keyterm, str) to treat strings as scalar,
otherwise iterate the sequence) and call query_params.add("keyterm", term) for
each element so multiple keyterm entries are added (keyterm=val1&keyterm=val2)
instead of encoding the whole list.
There was a problem hiding this comment.
@lukeocodes might be worth looking at.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Not only is it right, this file should have been updated by Fern.
Shit, i'll need to look into this
Uh oh!
There was an error while loading. Please reload this page.