Skip to content

feat: add parse_kv_pairs function to sessions utils #121

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.

Sign up for GitHub

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

Open
DeanChensj wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −0
Open

feat: add parse_kv_pairs function to sessions utils #121

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/google/adk_community/sessions/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,13 @@ def _json_serializer(obj):
if isinstance(obj, Decimal):
return float(obj)
return str(obj)

Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good practice to include unit tests for new utility functions to ensure they handle various edge cases (like empty strings, trailing commas, or spaces).

Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better type safety, you might want to use dict[str, str] as the return type.


def parse_kv_pairs(data: str) -> dict:
Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit tests for this new utility function to ensure it handles various input formats and edge cases correctly.

Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit tests for this new function in tests/unittests/sessions/test_utils.py (or a similar appropriate test file) to ensure it handles various input formats and edge cases correctly.

Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a more specific type hint like Dict[str, str] (imported from typing) to improve clarity on the return type.

"""Parses comma-separated key-value pairs into a dictionary."""
result = {}
parts = data.split(",")
Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If data has trailing commas or double commas, split(",") will produce empty strings, which will then cause part.split(":") to fail. You might want to filter out empty parts: parts = [p for p in data.split(",") if p.strip()].

Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail if any part doesn't contain exactly one colon (e.g., empty parts from trailing commas, or malformed input like "key1"). Consider adding a check or using part.partition(':') or part.split(':', 1) and validating the result.

Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will raise a ValueError if the input data is an empty string, because "".split(",") returns [''], and '' does not contain a colon. Consider adding a check for if not data: return {}.

Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If data is an empty string, data.split(",") will return ['']. This will cause an error on line 45 because an empty string doesn't contain a colon. Consider adding a check for empty input at the beginning of the function.

for part in parts:
Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If data is an empty string, data.split(",") results in ['']. This will lead to an error in the loop at the next line. Consider handling empty input at the beginning.

key, value = part.split(":")
Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using part.split(":", 1) to handle cases where the value might contain a colon (e.g., a URL). Currently, this will raise a ValueError: too many values to unpack in such cases.

Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is prone to ValueError. It will fail if a part doesn't contain a colon (e.g., "key") or contains multiple colons (e.g., "key:value:extra"). I recommend using part.split(":", 1) and checking if the result has exactly 2 elements, or using part.partition(":").

Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will raise a ValueError in two cases:

  1. If a part doesn't contain a colon (e.g., "key").
  2. If a part contains more than one colon (e.g., "url:https://google.com").

Consider using part.split(":", 1) to handle multiple colons and adding validation to handle missing colons gracefully.

result[key.strip()] = value.strip()
Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If data is an empty string, data.split(",") results in ['']. This will lead to an error in the loop. Consider handling empty input at the beginning.

Copy link
Copy Markdown

@github-actions github-actions Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail if part doesn't have a colon or has more than one. Using part.split(":", 1) would be safer if you only want the first colon to be the separator. You should also check if the split actually produced two parts.

return result
Loading