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

Add Messari crypto dd commands #1711

Merged
merged 22 commits into from
May 2, 2022
Merged

Add Messari crypto dd commands #1711

merged 22 commits into from
May 2, 2022

Conversation

jose-donato
Copy link
Contributor

@jose-donato jose-donato commented Apr 18, 2022

Description

WIP

How has this been tested?

  • Please describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.
  • Please also list any relevant details for your test configuration.

Checklist:

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My code passes all the checks pylint, flake8, black, ... To speed up development you should run pre-commit install.
  • New and existing unit tests pass locally with my changes. You can test this locally using pytest tests/....

@jose-donato jose-donato added the feat L Large T-Shirt size Feature label Apr 18, 2022
@@ -37,3 +37,12 @@ Coinbase
stats show coin stats
Messari
mcapdom show market cap dominance
mt show messari timeseries
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need for the "show" on the description in all of these

@minhhoang1023
Copy link
Contributor

Excellent work 🚀 ✨ This totally revamped the DD menu

@minhhoang1023
Copy link
Contributor

Was going through it and have a few comments:

Roadmap command
I remembered you mentioned the idea of plotting this as a plot, together with the price, to show how the news impacts the price. Shall we make it happen?

There's some href HTML tags left in the text
image

@minhhoang1023
Copy link
Contributor

For circulating supply, what do you think about adding the price indicator? If the project keeps on minting new tokens without increasing demand, price will inevitably decrease as per econ 101.

image

@minhhoang1023
Copy link
Contributor

minhhoang1023 commented Apr 20, 2022

For tk command, I got an error when loading ETH. Does it happen to you too?

image

Same with investors & fundraising & links command
image

image

image

@jose-donato jose-donato added feat XL Extra Large feature and removed feat L Large T-Shirt size Feature labels Apr 26, 2022
@jose-donato jose-donato marked this pull request as ready for review April 26, 2022 15:51
@minhhoang1023
Copy link
Contributor

Already said it, but I'm going to say it again, you're killing this @jose-donato - basically adding half of what we are having to the dd menu 🚀 From my interviews with users, they absolutely need these commands. Instead of having to navigate through multiple sites, now they can find it all in the terminal.

@minhhoang1023
Copy link
Contributor

A few small comments:

image

We should remove the list of time series in the mt command. The list is massive and i need to scroll a lot. Now that we have the query option, we no longer need this.

@minhhoang1023
Copy link
Contributor

minhhoang1023 commented Apr 27, 2022

NaT should be displayed as Unknown / or Not specified or sth humand-readable. i'll leave it up to you.

image

The href tag is still showing :( .

image

parser.add_argument(
"-q",
"--query",
type=str,
Copy link
Contributor

Choose a reason for hiding this comment

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

could we have it such that it will print out a table of possible time series similar to what we discussed before? Right now, it's not printing out anything, and still using the default of Transaction Volume

image

@@ -136,6 +139,8 @@ def __init__(
choices["twitter"]["-s"] = {
c: None for c in coinpaprika_view.TWEETS_FILTERS
}
choices["mt"] = {c: None for c in self.messari_timeseries}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it's possible to display the Title of Time Series for auto-completion instead of id? Then we can do the mapping later.

image

ax.plot(df.index, df["values"])

ax.set_title(f"{coin}'s {title}")
ax.set_ylabel(y_axis)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use the title as the plot title, to avoid the overflowing text
image

@@ -214,15 +218,18 @@ def get_close_price(asset: str, interval: str, since: int, until: int) -> pd.Dat
df = pd.DataFrame(json.loads(r.text))

if df.empty:
console.print(f"No data found for {asset} price.\n")
if print_errors:
console.print(f"No data found for {asset} price.\n")
else:
df = df.set_index("t")
df.index = pd.to_datetime(df.index, unit="s")

elif r.status_code == 401:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the Premium API Feat, instead of invalid API key.

We have the template on API error handling here - you might need to update depending on their status code

    if response.status_code == 200:
        d_data = response.json()
        if "economicCalendar" in d_data:
            df = pd.DataFrame(d_data["economicCalendar"])
        else:
            console.print("No latest economy calendar events found\n")
    elif response.status_code == 401:
        console.print("[red]Invalid API Key[/red]\n")
    elif response.status_code == 403:
        console.print("[red]API Key not authorized for Premium Feature[/red]\n")
    else:
        console.print(f"Error in request: {response.json()['error']}", "\n")

    return df

image

action="store_true",
help="Flag to show chart with coin price and roadmap dates",
dest="chart",
default=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

default should be True, so they can see your awesome chart ;)


if not df.empty:
df["Date"] = df["Date"].dt.date
print_rich_table(
Copy link
Contributor

Choose a reason for hiding this comment

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

Very small but this will add to user experience. It'd be nice to have this sort by date (descending) so we show the latest roadmap first, and have a limit flag. We could show top 5 as default, as otherwise it'd be rather long.

external_axes : Optional[List[plt.Axes]], optional
External axes (1 axis is expected in the list), by default None
"""
df, circ_df = get_tokenomics(coin, coingecko_symbol, circ_supply_src)
Copy link
Contributor

Choose a reason for hiding this comment

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

Got this error msg if it's from cg


@log_start_end(log=logger)
@check_api_key(["API_MESSARI_KEY"])
def display_fundraising(
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea why the investor % and org % doesn't match between the chart and text? With ETH it's matching tho so might be due to Messari itself?
image

@minhhoang1023
Copy link
Contributor

I also added a prettify_paragraph to make the text look nicer :D

Before:
image

After:
image

image

@Chavithra
Copy link
Contributor

Chavithra commented May 2, 2022

@jose-donato Looks like the tests fail was related to file naming : case sensitivity

@DidierRLopes DidierRLopes merged commit e3cd8ad into main May 2, 2022
@DidierRLopes DidierRLopes deleted the messari_dd branch May 30, 2022 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat XL Extra Large feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants