Skip to content
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

Fix SyntaxError: f-string: unmatched '{' error #1259

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

Fix SyntaxError: f-string: unmatched '{' error #1259

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
6 changes: 3 additions & 3 deletions python/assistant-stream/src/assistant_stream/serialization/data_stream.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ def encode_chunk(self, chunk: AssistantStreamChunk) -> str:
if chunk.type == "text-delta":
return f"0:{json.dumps(chunk.text_delta)}\n"
elif chunk.type == "tool-call-begin":
return f"b:{json.dumps({ "toolCallId": chunk.tool_call_id, "toolName": chunk.tool_name })}\n"
return f'b:{json.dumps({ "toolCallId": chunk.tool_call_id, "toolName": chunk.tool_name })}\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Fix potential f-string parsing issues

While changing quotes helps, the current implementation might still be prone to f-string parsing issues due to nested curly braces. A more robust approach would be to separate the JSON creation from the f-string interpolation.

Here's a clearer and more maintainable solution:

-            return f'b:{json.dumps({ "toolCallId": chunk.tool_call_id, "toolName": chunk.tool_name })}\n'
+            data = {"toolCallId": chunk.tool_call_id, "toolName": chunk.tool_name}
+            return f'b:{json.dumps(data)}\n'
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return f'b:{json.dumps({ "toolCallId": chunk.tool_call_id, "toolName": chunk.tool_name })}\n'
data = {"toolCallId": chunk.tool_call_id, "toolName": chunk.tool_name}
return f'b:{json.dumps(data)}\n'

elif chunk.type == "tool-call-delta":
return f"c:{json.dumps({ "toolCallId": chunk.tool_call_id, "argsTextDelta": chunk.args_text_delta })}\n"
return f'c:{json.dumps({ "toolCallId": chunk.tool_call_id, "argsTextDelta": chunk.args_text_delta })}\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Apply consistent f-string handling

Similar to the previous case, this line would benefit from separating the JSON structure from the f-string interpolation.

Apply this change for consistency:

-            return f'c:{json.dumps({ "toolCallId": chunk.tool_call_id, "argsTextDelta": chunk.args_text_delta })}\n'
+            data = {"toolCallId": chunk.tool_call_id, "argsTextDelta": chunk.args_text_delta}
+            return f'c:{json.dumps(data)}\n'
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return f'c:{json.dumps({ "toolCallId": chunk.tool_call_id, "argsTextDelta": chunk.args_text_delta })}\n'
data = {"toolCallId": chunk.tool_call_id, "argsTextDelta": chunk.args_text_delta}
return f'c:{json.dumps(data)}\n'

elif chunk.type == "tool-result":
return f"a:{json.dumps({ "toolCallId": chunk.tool_call_id, "result": chunk.result })}\n"
return f'a:{json.dumps({ "toolCallId": chunk.tool_call_id, "result": chunk.result })}\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Apply consistent f-string handling and consider refactoring

This line needs the same fix as the others. Additionally, given the similar pattern across all three changes, consider extracting this into a helper method.

Immediate fix:

-            return f'a:{json.dumps({ "toolCallId": chunk.tool_call_id, "result": chunk.result })}\n'
+            data = {"toolCallId": chunk.tool_call_id, "result": chunk.result}
+            return f'a:{json.dumps(data)}\n'

Consider this broader refactoring to reduce duplication:

def _encode_tool_data(self, prefix: str, data: dict) -> str:
    return f'{prefix}:{json.dumps(data)}\n'

Then use it like:

# Example usage:
return self._encode_tool_data('b', {
    "toolCallId": chunk.tool_call_id,
    "toolName": chunk.tool_name
})

Comment on lines +18 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Tests needed for data stream serialization

The codebase lacks test coverage for the data stream serialization functionality. You should add unit tests to verify:

  • Text delta encoding (type == "text-delta")
  • Tool call begin encoding (type == "tool-call-begin")
  • Tool call delta encoding (type == "tool-call-delta")
  • Tool result encoding (type == "tool-result")
  • Special character handling in all JSON fields
  • Proper newline handling in the encoded output
  • Media type return value
  • Async stream encoding
🔗 Analysis chain

Verify the changes with unit tests

While the syntax fixes look correct, please ensure these changes are covered by unit tests, especially testing with various types of tool names, IDs, and results that might contain special characters.

Let's check for existing tests:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Look for test files related to data_stream
fd -e py test_data_stream

Length of output: 27

Script:

#!/bin/bash
# Let's try a broader search for test files in the python/assistant-stream directory
fd -e py test python/assistant-stream

# Also search for imports of data_stream to see test coverage
rg "from assistant_stream.serialization.data_stream" -A 5

Length of output: 715

Script:

#!/bin/bash
# Let's search for test files in a different way and look for any tests related to serialization
rg -l "test.*serialization" python/assistant-stream/tests

# Also check if there are any test files directly in the package directory
fd -e py . python/assistant-stream/tests

# Let's also look at the actual implementation to understand the test requirements better
cat python/assistant-stream/src/assistant_stream/serialization/data_stream.py

Length of output: 1854

pass

def get_media_type(self) -> str:
Expand Down