feat: add parse_kv_pairs function to sessions utils#121
feat: add parse_kv_pairs function to sessions utils#121DeanChensj wants to merge 1 commit intogoogle:mainfrom
Conversation
|
@gemini-cli /review |
|
🤖 Hi @DeanChensj, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @DeanChensj, but I was unable to process your request. Please see the logs for more details. |
|
@gemini-cli /review |
|
🤖 Hi @DeanChensj, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
🤖 I'm sorry @DeanChensj, but I was unable to process your request. Please see the logs for more details. |
|
@gemini-cli /review |
|
🤖 Hi @DeanChensj, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return str(obj) | ||
|
|
||
|
|
||
| def parse_kv_pairs(data: str) -> dict: |
There was a problem hiding this comment.
Please add unit tests for this new utility function to ensure it handles various input formats and edge cases correctly.
| def parse_kv_pairs(data: str) -> dict: | ||
| """Parses comma-separated key-value pairs into a dictionary.""" | ||
| result = {} | ||
| parts = data.split(",") |
There was a problem hiding this comment.
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()].
| result = {} | ||
| parts = data.split(",") | ||
| for part in parts: | ||
| key, value = part.split(":") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you for the contribution! I've reviewed the code and found a few areas for improvement:
- Error Handling: The current implementation of
parse_kv_pairsis fragile. It will raise aValueErrorif the input string contains trailing commas (resulting in empty parts), missing colons, or multiple colons in a single pair. I recommend adding validation or using a more robust splitting strategy. - Missing Tests: No unit tests were provided for this new utility function. Please add tests covering both happy paths and edge cases (empty strings, malformed pairs, etc.).
- Typing: Precision in typing (e.g.,
dict[str, str]) would be beneficial.
Please address these comments.
| def parse_kv_pairs(data: str) -> dict: | ||
| """Parses comma-separated key-value pairs into a dictionary.""" | ||
| result = {} | ||
| parts = data.split(",") |
There was a problem hiding this comment.
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.
| if isinstance(obj, Decimal): | ||
| return float(obj) | ||
| return str(obj) | ||
|
|
There was a problem hiding this comment.
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).
| if isinstance(obj, Decimal): | ||
| return float(obj) | ||
| return str(obj) | ||
|
|
There was a problem hiding this comment.
For better type safety, you might want to use dict[str, str] as the return type.
There was a problem hiding this comment.
The parse_kv_pairs function is a useful addition, but it needs to be more robust to handle edge cases like empty strings, malformed input (missing colons), and values that might contain colons. Additionally, please include unit tests to verify the functionality and prevent regressions.
| result = {} | ||
| parts = data.split(",") | ||
| for part in parts: | ||
| key, value = part.split(":") |
There was a problem hiding this comment.
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(":").
| def parse_kv_pairs(data: str) -> dict: | ||
| """Parses comma-separated key-value pairs into a dictionary.""" | ||
| result = {} | ||
| parts = data.split(",") |
There was a problem hiding this comment.
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 {}.
| return str(obj) | ||
|
|
||
|
|
||
| def parse_kv_pairs(data: str) -> dict: |
There was a problem hiding this comment.
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.
|
@gemini-cli /review |
|
🤖 Hi @DeanChensj, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
Thanks for the contribution! The parse_kv_pairs function is a useful addition, but it needs some robustness improvements to handle edge cases and malformed input. Specifically:
- Error Handling: The current implementation will crash if a part doesn't contain a colon or contains multiple colons.
- Empty Input: An empty string will also cause a crash.
- Type Hinting: More specific type hints would be beneficial.
Please see the line-specific comments for suggestions.
| def parse_kv_pairs(data: str) -> dict: | ||
| """Parses comma-separated key-value pairs into a dictionary.""" | ||
| result = {} | ||
| parts = data.split(",") |
There was a problem hiding this comment.
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.
| result = {} | ||
| parts = data.split(",") | ||
| for part in parts: | ||
| key, value = part.split(":") |
There was a problem hiding this comment.
This line will raise a ValueError in two cases:
- If a part doesn't contain a colon (e.g.,
"key"). - 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.
| return str(obj) | ||
|
|
||
|
|
||
| def parse_kv_pairs(data: str) -> dict: |
There was a problem hiding this comment.
Consider using a more specific type hint like Dict[str, str] (imported from typing) to improve clarity on the return type.
|
🤖 Hi @DeanChensj, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
| parts = data.split(",") | ||
| for part in parts: | ||
| key, value = part.split(":") | ||
| result[key.strip()] = value.strip() |
There was a problem hiding this comment.
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.
| """Parses comma-separated key-value pairs into a dictionary.""" | ||
| result = {} | ||
| parts = data.split(",") | ||
| for part in parts: |
There was a problem hiding this comment.
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.
| parts = data.split(",") | ||
| for part in parts: | ||
| key, value = part.split(":") | ||
| result[key.strip()] = value.strip() |
There was a problem hiding this comment.
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.
1 participant
No description provided.