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

refactoring load, changed chart to candle #1838

Merged
merged 22 commits into from
Jun 9, 2022
Merged

Conversation

jose-donato
Copy link
Contributor

@jose-donato jose-donato commented May 19, 2022

DRAFT: WILL BE UPDATED SOON

  • load simplified
  • changed command chart to candle to match stocks

image

@jose-donato jose-donato added the refactor Refactor code label May 19, 2022
@minhhoang1023
Copy link
Contributor

Trying out load BTC and not working for me:

image

I'd suggest having load bitcoin enabled too. A lot of people will use this like this

@minhhoang1023
Copy link
Contributor

minhhoang1023 commented Jun 2, 2022

Nice work! 🚀 This is a much-needed one.

Some comments:

  1. We should remove the auto-completion with source

image

  1. When some symbols do not exist, this errors throws :(

image

  1. Super small but my OCD cannot ignore this. Could we capitalize the Symbol?

image

@minhhoang1023
Copy link
Contributor

Tested the dd commands with breaking changes and worked wonderfully!

@jose-donato
Copy link
Contributor Author

Nice :) will go through your comments today

@minhhoang1023
Copy link
Contributor

@jose-donato Let me know when it's ready and I'll review it. Let's get this bad boy out!

Now the load is not working at all..

image

@minhhoang1023
Copy link
Contributor

minhhoang1023 commented Jun 7, 2022

When I loaded ADA in upper case, it said coin is not found. But in lower case that'd work. And circulating supply shouldn't be 0.

image

Same with AVAX
image

@minhhoang1023
Copy link
Contributor

When the starting date is specified, an error was thrown

image

@jose-donato
Copy link
Contributor Author

thanks for the help @Chavithra

@jose-donato jose-donato marked this pull request as ready for review June 9, 2022 08:33
@minhhoang1023
Copy link
Contributor

When I use the load multiple times, coingecko would limit # of API calls. We should have an error message telling them that they should try again in a bit. Other than that, work perfectly! ✨

image

@jose-donato jose-donato merged commit 0e03b9e into main Jun 9, 2022
@jose-donato jose-donato deleted the refactor/crypto_load branch June 9, 2022 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants