-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Data Client #1147
base: master
Are you sure you want to change the base?
Data Client #1147
Conversation
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.
Just curious why redeclare
klines_df = historical_klines_response_df
tests/test_historical_klines.py
Outdated
klines = historical_klines_response | ||
assert len(klines) == 500 |
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.
klines = historical_klines_response | |
assert len(klines) == 500 | |
assert len(historical_klines_response) == 500 |
def test_historical_klines(historical_klines_response_df): | ||
klines_df = historical_klines_response_df | ||
assert len(klines_df) == 500 | ||
assert isinstance(klines_df, DataFrame) |
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.
Nice!
Hi @mrbaguvix thanks for the review.
This was mostly for brevity in reading the test scenarios. But it's not necessary and the long variable name can be used instead. |
extra-requirements.txt
Outdated
@@ -0,0 +1 @@ | |||
pandas |
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.
I think, you should define version range: pandas>=n.n.n<n+1
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
done
|
||
import pytest | ||
import requests_mock | ||
|
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.
Are you shure, that this file should be stored in project root?
And add module's description
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.
Confest in the root directory is a best practice to help us avoid adding init files to the test directory. It facilities local imports, especially as they pertain to testing.
https://stackoverflow.com/questions/34466027/in-pytest-what-is-the-use-of-conftest-py-files
Test root path: This is a bit of a hidden feature. By defining conftest.py in your root path, you will have pytest recognizing your application modules without specifying PYTHONPATH. In the background, py.test modifies your sys.path by including all submodules which are found from the root path.
Provides users with an option to return client responses in
pandas.DataFrame
format.Supercedes #1133