-
Notifications
You must be signed in to change notification settings - Fork 806
Check the python format for adk samples #560
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
|
No functional changes. Only formatting the ADK samples to reduce diff size for future commits. |
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.
Code Review
This pull request applies pyink formatting across the Python sample files, which is a great step for code consistency. The changes are mostly stylistic, involving indentation and quote style.
I've added a few comments on areas that could be improved for better maintainability and correctness:
- Replacing magic strings (hardcoded URLs) with constants.
- Refactoring a large, hardcoded JSON string into a more manageable form.
- Improving the robustness of deep dictionary access.
- Correcting a typo in a method name.
Overall, the formatting changes look good. Addressing the suggested improvements will further enhance the code quality.
| with open(file_path) as f: | ||
| contact_data_str = f.read() | ||
| if base_url := tool_context.state.get("base_url"): | ||
| contact_data_str = contact_data_str.replace("http://localhost:10002", base_url) |
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.
The hardcoded URL "http://localhost:10002" is a magic string. It's better to define it as a constant at the module level (e.g., DEFAULT_DATA_BASE_URL = "http://localhost:10002") to improve readability and maintainability. This makes it easier to find and update if the default URL changes.
| contact_data_str = contact_data_str.replace("http://localhost:10002", base_url) | |
| contact_data_str = contact_data_str.replace(DEFAULT_DATA_BASE_URL, base_url) |
| json_content = ( | ||
| '[{ "beginRendering": { "surfaceId": "action-modal", "root":' | ||
| ' "modal-wrapper" } }, { "surfaceUpdate": { "surfaceId": "action-modal",' | ||
| ' "components": [ { "id": "modal-wrapper", "component": { "Modal": {' | ||
| ' "entryPointChild": "hidden", "contentChild": "msg", "open": true } } },' | ||
| ' { "id": "hidden", "component": { "Text": { "text": {"literalString": "' | ||
| ' "} } } }, { "id": "msg", "component": { "Text": { "text":' | ||
| ' {"literalString": "Message Sent (Fallback)"} } } } ] } }]' | ||
| ) |
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.
| SUPPORTED_CONTENT_TYPES = ["text", "text/plain"] | ||
|
|
||
| @classmethod | ||
| async def programmtically_route_user_action_to_subagent( |
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.
| server_to_client_json["properties"]["surfaceUpdate"]["properties"]["components"][ | ||
| "items" | ||
| ]["properties"]["component"]["properties"] = standard_catalog_json |
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.
| restaurant_data_str = restaurant_data_str.replace( | ||
| "http://localhost:10002", base_url | ||
| ) |
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.
The hardcoded URL "http://localhost:10002" is a magic string. It's better to define it as a constant at the module level (e.g., DEFAULT_DATA_BASE_URL = "http://localhost:10002") to improve readability and maintainability. This makes it easier to find and update if the default URL changes.
restaurant_data_str = restaurant_data_str.replace(DEFAULT_DATA_BASE_URL, base_url)| a2ui_schema_json["properties"]["surfaceUpdate"]["properties"]["components"][ | ||
| "items" | ||
| ]["properties"]["component"]["properties"] = catalog_json |
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.
Description
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. For larger changes, raising an issue first helps reduce redundant work.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.