Skip to content
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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Data Client #1147

wants to merge 9 commits into from

Conversation

s2t2
Copy link
Contributor

@s2t2 s2t2 commented Feb 19, 2022

Provides users with an option to return client responses in pandas.DataFrame format.

Supercedes #1133

@s2t2 s2t2 mentioned this pull request Feb 19, 2022
@s2t2
Copy link
Contributor Author

s2t2 commented Feb 19, 2022

Screen Shot 2022-02-19 at 12 46 59 PM

Copy link

@0ex-d 0ex-d left a 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

Comment on lines 14 to 15
klines = historical_klines_response
assert len(klines) == 500
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@s2t2
Copy link
Contributor Author

s2t2 commented Feb 20, 2022

Hi @mrbaguvix thanks for the review.

Just curious why redeclare klines_df = historical_klines_response_df

This was mostly for brevity in reading the test scenarios. But it's not necessary and the long variable name can be used instead.

@@ -0,0 +1 @@
pandas

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +1 to +4

import pytest
import requests_mock

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

Copy link
Contributor Author

@s2t2 s2t2 Mar 21, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants