-
Notifications
You must be signed in to change notification settings - Fork 4.7k
[Bug] Python Error Fix β Type Hint Compatibility #249
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
Summary of ChangesHello @samchenku, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This PR fixes Python type hint compatibility issues in Alpha Vantage dataflow modules by replacing the PEP 604 union operator ( Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with π and π on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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 correctly addresses the type hint compatibility issue for Python versions below 3.10 by replacing the | operator with typing.Union. I've identified a few areas where the type hints can be further improved for correctness and consistency. My comments focus on ensuring the type annotations accurately reflect what the functions return and improving code style according to PEP 8.
| pass | ||
|
|
||
| def _make_api_request(function_name: str, params: dict) -> dict | str: | ||
| def _make_api_request(function_name: str, params: dict) -> Union[dict, str]: |
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 function implementation shows that it always returns response_text, which is a string. It never returns a dictionary, as the JSON is only parsed to check for API errors before being discarded. Therefore, the return type hint should be str instead of Union[dict, str] to accurately reflect the function's behavior and prevent confusion for developers and static analysis tools.
| def _make_api_request(function_name: str, params: dict) -> Union[dict, str]: | |
| def _make_api_request(function_name: str, params: dict) -> str: |
| from typing import Union | ||
|
|
||
| def get_news(ticker, start_date, end_date) -> dict[str, str] | str: | ||
| def get_news(ticker, start_date, end_date) -> Union[dict[str, str], str]: |
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.
This function returns the raw string response from _make_api_request. It does not parse the JSON, so it never returns a dict. The return type hint should be str to reflect this.
Additionally, to improve code clarity and maintainability, it's good practice to add type hints for the function's parameters.
| def get_news(ticker, start_date, end_date) -> Union[dict[str, str], str]: | |
| def get_news(ticker: str, start_date: str, end_date: str) -> str: |
| return _make_api_request("NEWS_SENTIMENT", params) | ||
|
|
||
| def get_insider_transactions(symbol: str) -> dict[str, str] | str: | ||
| def get_insider_transactions(symbol: str) -> Union[dict[str, str], str]: |
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.
Similar to get_news, this function returns a string from _make_api_request, not a dictionary. The return type hint should be str to accurately represent the function's output.
| def get_insider_transactions(symbol: str) -> Union[dict[str, str], str]: | |
| def get_insider_transactions(symbol: str) -> str: |
| @@ -1,6 +1,7 @@ | |||
| from .alpha_vantage_common import _make_api_request, format_datetime_for_api | |||
| from typing import Union | |||
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.
According to the PEP 8 style guide, imports should be grouped in the following order: standard library, third-party, and then local application imports. typing is a standard library module and should be imported before local imports like .alpha_vantage_common.
To fix this, you would move this line above the import on line 1. Due to tooling limitations, I cannot provide an automated suggestion that spans both lines, but the correct order would be:
from typing import Union
from .alpha_vantage_common import _make_api_request, format_datetime_for_api
π Python Error Fix β Type Hint Compatibility
Summary
This pull request fixes Python type hint compatibility issues in the Alpha Vantage dataflow modules.
It replaces the use of the PEP 604 union operator (
|) withtyping.Union, ensuring compatibility with Python versions below 3.10 and improving type consistency across the codebase.Changes
1.
tradingagents/dataflows/alpha_vantage_common.pyAdded missing import:
Updated function signature:
(Previously used
dict | str)2.
tradingagents/dataflows/alpha_vantage_news.pyAdded missing import:
Updated function signatures:
Why This Fix Matters
mypy,pyright).Testing
Reviewer note: This is a non-functional change focused purely on type hint and compatibility cleanup.