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

Fixing default view and ascending order in cglosers and cggainers #1923

Merged

Conversation

simmonsj330
Copy link
Contributor

@simmonsj330 simmonsj330 commented Jun 9, 2022

Description

Fixes #1905
Made default cglosers sort by Market Cap [$] and switched ordering of sorting from descending to ascending.
Screen Shot 2022-06-09 at 5 30 37 PM
Screen Shot 2022-06-09 at 5 30 49 PM

  • Summary of the change / bug fix.
  • Link # issue, if applicable.
  • Screenshot of the feature or the bug before/after fix, if applicable.
  • Relevant motivation and context.

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/....

@simmonsj330 simmonsj330 added the bug Fix bug label Jun 9, 2022
@jmaslek
Copy link
Collaborator

jmaslek commented Jun 10, 2022

hmmm. The market cap still sorts in reverse order

2022 Jun 10, 09:15 (🦋) /crypto/disc/ $ cggainers -s Market Cap [$]

┏━━━━━━━━┳━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Symbol ┃ Name              ┃ Price [$] ┃ Market Cap [$] ┃ Market Cap Rank ┃ Volume [$] ┃ Change 1h [%] ┃
┡━━━━━━━━╇━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ dai    │ Dai               │ 1.00      │ 6.6B           │ 15              │ 204.6M     │ -0.08         │
├────────┼───────────────────┼───────────┼────────────────┼─────────────────┼────────────┼───────────────┤
│ steth  │ Lido Staked Ether │ 1665.17   │ 7B             │ 14              │ 167.9M     │ -2.43         │
├────────┼───────────────────┼───────────┼────────────────┼─────────────────┼────────────┼───────────────┤

Also having to have the user type out Market Cap [$] is tedious, and should be able to sort by Market Cap (then we can add in the ($) after the fact.

You are also getting a warning which can be directly applied to pycoingecko_model (df = pd.concat([df, pd.DataFrame(data)]) instead of using append)

@DidierRLopes
Copy link
Collaborator

After doing James requests, for the test to pass you need to do:
pytest tests/openbb_terminal/cryptocurrency/discovery/test_pycoingecko_view.py --record-mode=rewrite

@simmonsj330
Copy link
Contributor Author

@jmaslek Sorting is in ascending order where the lowest market cap would put it at the highest market cap rank. For example bitcoin is the 1 market cap rank and appears at the top when sorting by rank but appears at the bottom when sorting by market cap itself

@jmaslek
Copy link
Collaborator

jmaslek commented Jun 15, 2022

@jmaslek Sorting is in ascending order where the lowest market cap would put it at the highest market cap rank. For example bitcoin is the 1 market cap rank and appears at the top when sorting by rank but appears at the bottom when sorting by market cap itself

I see. One could technically sort on the negative rank, but I think the user should understand why the sorting is in reverse order for the rank. So just add the test and you are good to go!

@DidierRLopes DidierRLopes merged commit 2a5fbd4 into OpenBB-finance:main Jun 24, 2022
@simmonsj330 simmonsj330 deleted the cgloser_marketcap_order_fix branch June 29, 2022 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] crypto/disc/cglosers - sorting by market cap is not on by default, as described by the help dialogue.
3 participants