-
Notifications
You must be signed in to change notification settings - Fork 111
fix: resolves issues with String|String [] encoding #636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes extend four query parameters ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/deepgram/listen/v2/client.py (1)
46-47: Multi-valuekeytermhandling is correct and backward compatible; docstring slightly out of syncThe widened type and
isinstance(keyterm, str) / else: for term in keytermpattern correctly:
- preserves single-string behavior,
- expands sequences into repeated
keyterm=...entries, and- avoids accidentally iterating over characters for plain strings.
Only minor nit: the
keytermdocstrings still statetyping.Optional[str], which no longer matches the annotated type. If these docstrings are not fully generator-owned, consider updating them (or the generator) to reflectOptional[Union[str, Sequence[str]]].Also applies to: 102-107, 161-162, 217-222
src/deepgram/listen/v2/raw_client.py (1)
34-35: Raw V2keytermmulti-value support matches client behavior; consider docstring alignmentThe raw sync/async V2 connect methods now mirror the high-level client behavior:
- strings yield a single
keytermquery param,- sequences yield multiple
keytermparams, one per item.This fixes the list-encoding issue while maintaining compatibility for existing string callers. As with the client, the docstrings for
keytermstill advertisetyping.Optional[str]; if these docs aren’t purely generated, consider updating them (or the generator) to describe theOptional[Union[str, Sequence[str]]]input.Also applies to: 90-95, 138-139, 194-199
src/deepgram/listen/v1/client.py (1)
56-57: V1 multi-value handling forkeyterm/keywords/replace/searchlooks good; docs lag the typesThe updated signatures and query-param logic for these four fields in both
V1Client.connectandAsyncV1Client.connectcorrectly:
- Preserve old behavior for single strings.
- Expand sequences into repeated query params (e.g.,
search=foo&search=bar).- Avoid treating strings as sequences of characters via the
isinstance(..., str)guard.- Keep sync/async behavior consistent.
The effective behavior change for empty lists (now emitting no param rather than a stringified list) is an improvement on the previous broken encoding.
Only small suggestion: the parameter docstrings for
keyterm,keywords,replace, andsearchstill saytyping.Optional[str]; if feasible, consider updating the generator or docs to reflectOptional[Union[str, Sequence[str]]]so users see the multi-value capability in generated documentation.Also applies to: 66-68, 169-180, 197-210, 283-285, 293-295, 396-407, 424-437
src/deepgram/listen/v1/raw_client.py (1)
37-38: Raw V1 multi-value parameter behavior is consistent and correct; docstrings could be updatedThe raw v1 sync and async connect methods now:
- Accept
Optional[Union[str, Sequence[str]]]forkeyterm,keywords,replace, andsearch.- Correctly emit one query param per value when a sequence is provided, while keeping legacy single-string usage intact.
- Mirror the high-level V1 client behavior so there’s no divergence between raw and wrapped APIs.
As elsewhere, the docstrings for these parameters still show
typing.Optional[str]. If you control the generator or these docs, it would be a small win to update them to reflect the union type so that SDK users discover the new multi-value support from the documentation as well.Also applies to: 47-49, 150-161, 178-191, 243-245, 253-255, 356-367, 384-397
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/deepgram/listen/v1/client.py(8 hunks)src/deepgram/listen/v1/raw_client.py(8 hunks)src/deepgram/listen/v2/client.py(4 hunks)src/deepgram/listen/v2/raw_client.py(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-07-01T19:17:04.194Z
Learnt from: dvonthenen
Repo: deepgram/deepgram-python-sdk PR: 426
File: deepgram/clients/listen/v1/__init__.py:36-43
Timestamp: 2024-07-01T19:17:04.194Z
Learning: Unused imports in `deepgram/clients/listen/v1/__init__.py` are retained to maintain backward compatibility and should not be flagged for removal in reviews.
Applied to files:
src/deepgram/listen/v1/client.pysrc/deepgram/listen/v1/raw_client.py
📚 Learning: 2024-10-09T02:19:46.087Z
Learnt from: dvonthenen
Repo: deepgram/deepgram-python-sdk PR: 426
File: deepgram/clients/listen/v1/rest/options.py:12-12
Timestamp: 2024-10-09T02:19:46.087Z
Learning: Unused imports in `deepgram/clients/listen/v1/rest/options.py` are retained to maintain backwards compatibility.
Applied to files:
src/deepgram/listen/v1/client.py
📚 Learning: 2024-07-01T19:21:39.778Z
Learnt from: dvonthenen
Repo: deepgram/deepgram-python-sdk PR: 426
File: deepgram/clients/listen/v1/websocket/__init__.py:8-8
Timestamp: 2024-07-01T19:21:39.778Z
Learning: Unused imports in `deepgram/clients/listen/v1/websocket/__init__.py` are retained to maintain backward compatibility and should not be flagged for removal in reviews.
Applied to files:
src/deepgram/listen/v1/raw_client.py
📚 Learning: 2025-06-22T17:02:32.416Z
Learnt from: lukeocodes
Repo: deepgram/deepgram-python-sdk PR: 543
File: tests/unit_test/test_unit_authentication.py:67-87
Timestamp: 2025-06-22T17:02:32.416Z
Learning: In the Deepgram Python SDK, DeepgramClient("arbitrary-string") should be treated as an API key for backward compatibility with existing code patterns.
Applied to files:
src/deepgram/listen/v2/raw_client.py
PR Summary
Fix: String|String[] parameter URL encoding for WebSocket clients
Issue: #629
Problem:
When passing list values to
keyterm,keywords,replace, andsearchparameters in WebSocket clients, the SDK incorrectly URL-encoded the entire list as a string (e.g.,keyterm=%5B%27TERM1%27%2C+%27TERM2%27%5D) instead of creating multiple query parameters (e.g.,keyterm=TERM1&keyterm=TERM2).Solution:
typing.Union[str, typing.Sequence[str]]Files Changed:
src/deepgram/listen/v1/client.py- Fixed: keyterm, keywords, replace, searchsrc/deepgram/listen/v1/raw_client.py- Fixed: keyterm, keywords, replace, searchsrc/deepgram/listen/v2/client.py- Fixed: keytermsrc/deepgram/listen/v2/raw_client.py- Fixed: keytermTesting:
Note: REST/media API clients already handled this correctly; only WebSocket clients were affected.
Reproduction Script used for testing
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.