-
Notifications
You must be signed in to change notification settings - Fork 188
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
Tradier data source #335
Tradier data source #335
Changes from 2 commits
042df10
2da9c1e
e68b552
6d262ff
a35b35c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
from pathlib import Path | ||
|
||
from dotenv import load_dotenv | ||
|
||
# Load sensitive information such as API keys from .env files so that they are not stored in the repository | ||
# but can still be accessed by the tests through os.environ | ||
secrets_path = Path(__file__).parent.parent / '.secrets' | ||
if secrets_path.exists(): | ||
for secret_file in secrets_path.glob('*.env'): | ||
load_dotenv(secret_file) | ||
Comment on lines
+8
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Comment on lines
+9
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Suggested change
|
||||||
assert round(results["volatility"] * 100, 0) == 20.0 | ||||||
assert round(results["total_return"] * 100, 1) == 5.3 | ||||||
assert round(results["max_drawdown"]["drawdown"] * 100, 1) == 0.0 | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,13 +1,35 @@ | ||||||||||||||||||||||
import os | ||||||||||||||||||||||
|
||||||||||||||||||||||
import pytest | ||||||||||||||||||||||
|
||||||||||||||||||||||
from lumibot.brokers import Tradier | ||||||||||||||||||||||
from lumibot.data_sources import TradierData | ||||||||||||||||||||||
from lumibot.entities import Asset | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
||||||||||||||||||||||
TRADIER_ACCOUNT_ID_PAPER = os.getenv("TRADIER_ACCOUNT_ID_PAPER") | ||||||||||||||||||||||
TRADIER_TOKEN_PAPER = os.getenv("TRADIER_TOKEN_PAPER") | ||||||||||||||||||||||
Comment on lines
+9
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Suggested change
|
||||||||||||||||||||||
|
||||||||||||||||||||||
|
||||||||||||||||||||||
@pytest.fixture | ||||||||||||||||||||||
def tradier_ds(): | ||||||||||||||||||||||
return TradierData(TRADIER_ACCOUNT_ID_PAPER, TRADIER_TOKEN_PAPER, True) | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
||||||||||||||||||||||
@pytest.mark.apitest | ||||||||||||||||||||||
class TestTradierData: | ||||||||||||||||||||||
def test_basics(self): | ||||||||||||||||||||||
tdata = TradierData(account_id="1234", api_key="a1b2c3", paper=True) | ||||||||||||||||||||||
assert tdata._account_id == "1234" | ||||||||||||||||||||||
|
||||||||||||||||||||||
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 | ||||||||||||||||||||||
Comment on lines
+24
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Suggested change
|
||||||||||||||||||||||
|
||||||||||||||||||||||
|
||||||||||||||||||||||
@pytest.mark.apitest | ||||||||||||||||||||||
class TestTradierBroker: | ||||||||||||||||||||||
def test_basics(self): | ||||||||||||||||||||||
broker = Tradier(account_id="1234", api_token="a1b2c3", paper=True) | ||||||||||||||||||||||
|
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 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.