Tradier data source #335
Tradier data source #335
Conversation
davidlatte
had a problem deploying
to
unit-tests
GitHub Actions
Failure
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
The Korbit AI Mentor has reviewed your code. To discuss an issue, reply using the tag @korbit-ai-mentor.
Please leave a 👍 if this is helpful and 👎 if it is irrelevant.
lumibot/data_sources/__init__.py
Outdated
| TradierAPIError, | ||
| TradierData, | ||
| ) | ||
| from .tradier_data import TradierData |
There was a problem hiding this comment.
The import statement for the TradierData class has been modified to exclude the import of several constants and the TradierAPIError class. If these constants and the TradierAPIError class are used elsewhere in the code, this could potentially lead to a NameError.
| def get_last_price(self, asset, quote=None, exchange=None): | ||
| pass | ||
| """ | ||
| This function returns the last price of an asset. | ||
| Parameters | ||
| ---------- | ||
| asset | ||
| quote | ||
| exchange | ||
|
|
||
| Returns | ||
| ------- | ||
| float | ||
| Price of the asset | ||
| """ | ||
| price = self.tradier.market.get_last_price(asset.symbol) | ||
| return price |
There was a problem hiding this comment.
The get_last_price method does not handle any exceptions. If the Tradier API fails to return a price, the method will crash. It's important to handle exceptions and provide a meaningful error message to the user.
| TRADIER_ACCOUNT_ID_PAPER = os.getenv("TRADIER_ACCOUNT_ID_PAPER") | ||
| TRADIER_TOKEN_PAPER = os.getenv("TRADIER_TOKEN_PAPER") |
There was a problem hiding this comment.
The environment variables 'TRADIER_ACCOUNT_ID_PAPER' and 'TRADIER_TOKEN_PAPER' are being fetched directly using os.getenv() without any default value. If these environment variables are not set, the os.getenv() function will return None, which could lead to unexpected behavior or errors. It's a good practice to provide a default value when using os.getenv() to prevent such issues.
| TRADIER_ACCOUNT_ID_PAPER = os.getenv("TRADIER_ACCOUNT_ID_PAPER") | |
| TRADIER_TOKEN_PAPER = os.getenv("TRADIER_TOKEN_PAPER") | |
| TRADIER_ACCOUNT_ID_PAPER = os.getenv('TRADIER_ACCOUNT_ID_PAPER', 'default_account_id') | |
| TRADIER_TOKEN_PAPER = os.getenv('TRADIER_TOKEN_PAPER', 'default_token') |
| @@ -134,7 +134,7 @@ def test_stock_diversified_leverage(self): | |||
| assert isinstance(strat_obj, DiversifiedLeverage) | |||
|
|
|||
| # Check that the results are correct | |||
| assert round(results["cagr"] * 100, 1) == 1231963.9 | |||
| assert round(results["cagr"] * 100, 1) >= 1231000.0 | |||
There was a problem hiding this comment.
The test assertion for the 'cagr' result in the 'test_stock_diversified_leverage' test has been changed from a fixed value to a range. This could potentially lead to false positives in the test results, as the test would pass even if the 'cagr' result is lower than expected. It would be better to keep the assertion as a fixed value to ensure that the test accurately verifies the expected behavior of the code.
| assert round(results["cagr"] * 100, 1) >= 1231000.0 | |
| assert round(results['cagr'] * 100, 1) == 1231963.9 |
| if secrets_path.exists(): | ||
| for secret_file in secrets_path.glob('*.env'): | ||
| load_dotenv(secret_file) |
There was a problem hiding this comment.
While it's good practice to keep sensitive information like API keys out of the repository, the current implementation could potentially lead to unexpected behavior if the .secrets directory or any .env files within it are missing. It would be beneficial to add error handling to alert the user if the expected .env files are not found.
| def test_get_last_price(self, tradier_ds): | ||
| asset = Asset("AAPL") | ||
| price = tradier_ds.get_last_price(asset) | ||
| assert isinstance(price, float) | ||
| assert price > 0.0 |
There was a problem hiding this comment.
The test 'test_get_last_price' assumes that the price of the asset 'AAPL' will always be greater than 0.0. While this is likely to be true for a real stock like 'AAPL', it's not a good practice to make such assumptions in unit tests. Instead, the test should be designed in a way that it can handle any possible return value from the 'get_last_price' method.
| def test_get_last_price(self, tradier_ds): | |
| asset = Asset("AAPL") | |
| price = tradier_ds.get_last_price(asset) | |
| assert isinstance(price, float) | |
| assert price > 0.0 | |
| def test_get_last_price(self, tradier_ds): | |
| asset = Asset('AAPL') | |
| price = tradier_ds.get_last_price(asset) | |
| assert isinstance(price, float) | |
| assert price >= 0.0 |
| for secret_file in secrets_path.glob('*.env'): | ||
| load_dotenv(secret_file) |
There was a problem hiding this comment.
The current implementation loads all .env files in the .secrets directory. This could lead to unexpected behavior if there are .env files in the .secrets directory that are not intended to be loaded for the tests. It would be better to specify the exact .env files to load.
… lumi_tradier gets released to PyPi
davidlatte
had a problem deploying
to
unit-tests
GitHub Actions
Failure
davidlatte
temporarily deployed
to
unit-tests
GitHub Actions
Inactive
davidlatte
temporarily deployed
to
unit-tests
GitHub Actions
Inactive
Tradier DataSource working with get_last_price() for a real stock. Unittest implemented and fully plumbed in lumibot, but these are now marked as an "apitest" which GitHub CI/CD pipelines will skip to avoid hitting any kind of rate limits or requiring separate Unit Test Tradier accounts.