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

Decorate OHLC data #1133

Closed
wants to merge 6 commits into from
Closed

Decorate OHLC data #1133

wants to merge 6 commits into from

Conversation

s2t2
Copy link
Contributor

@s2t2 s2t2 commented Jan 30, 2022

Implements an output_format parmater for the Client.get_historical_klines function:

  • By default, returns a list of lists (current behavior).
  • With output_format="df", returns a pandas.DataFrame
  • With output_format="decorated", returns a list of decorated value objects.

@s2t2
Copy link
Contributor Author

s2t2 commented Jan 30, 2022

Not sure if CI runs on forks, but here are the results of local tests:

Screen Shot 2022-01-29 at 8 49 03 PM

@sammchardy
Copy link
Owner

Hi @s2t2 I do like this option to update the output, dataframes would be useful for some use cases.

I don't want to have pandas as a default requirement and increase the library size.

Could we make pandas an optional import or move the convertors to utility functions that can convert the output of any kline function. Then if someone wants to use it they can install pandas separately, or with a pip install python-binance[pandas] option. Open to suggestions.

@s2t2
Copy link
Contributor Author

s2t2 commented Feb 13, 2022

Hi @sammchardy thanks for your review!

The optional dependency pip install python-binance[pandas] sounds like a good solution, to provide users with maximum flexibility. I haven't done one before but it seems straightforward.

Then we'll need to leave the underlying functions untouched and add separate decorator functions. Or maybe a higher-level service-style class.

I'd be happy to work on incorporating these changes.

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

s2t2 commented Feb 19, 2022

@sammchardy FYI I have implemented these changes in #1147

closing this PR in favor of that one.

@s2t2 s2t2 closed this Feb 19, 2022
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.

2 participants