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

Bug/crypto defi vaults command broken #1822 #1874

Conversation

JakubPluta
Copy link
Contributor

Description

  • Summary of the change / bug fix.
    Coindix changed their API spec. Now endpoint https://apiv2.coindix.com/search doesn't take as query parameter risk keyword, and also it's not returned in response. Also as get request to mentioned endpoint has flag:
    response = requests.get(..., verify=False) it screams with urllib3 warnings. That`s why I also disabled this warning in stdout. For now it's in
  • Link # issue, if applicable.
    [Bug] Crypto/DeFi/Vaults - Command appears to be broken #1822
  • Screenshot of the feature or the bug before/after fix, if applicable.
    Before fix:
    image

After fix.
image

How has this been tested?

  • I updated test cassetes, txt's, and csv's + i run manually tests.

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

import pandas as pd
import requests

from openbb_terminal.decorators import log_start_end
from openbb_terminal.helper_funcs import get_user_agent


urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To disable Insecure Request Warning I disabled it on coindix_model level, but maybe it should be moved to some more flags/config related place ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For now I think this looks good yea!

@DidierRLopes DidierRLopes added the bug Fix bug label May 26, 2022
@DidierRLopes
Copy link
Collaborator

Welcome back @JakubPluta 🚀

For the tests you can do

pytest tests/openbb_terminal/cryptocurrency/test_cryptocurrency_helpers.py --record-mode=rewrite

@JakubPluta
Copy link
Contributor Author

Welcome back @JakubPluta 🚀

For the tests you can do

pytest tests/openbb_terminal/cryptocurrency/test_cryptocurrency_helpers.py --record-mode=rewrite

Thanks Didier, I have to get used to the current structure of the project, as it changed a "little" from my last PR :)

@piiq
Copy link
Contributor

piiq commented May 30, 2022

the tests should pass once this #1885 is merged

@DidierRLopes DidierRLopes merged commit b9e097e into OpenBB-finance:main Jun 1, 2022
deeleeramone pushed a commit to deeleeramone/OpenBBTerminal that referenced this pull request Jun 1, 2022
…ance#1874)

* Add separate view for finbrain for crypto curreny sentiment analysis

* Move json with symbols to separate directory, little refactoring, update readme

* Add screenshot of finbrain for PolkaDot

* Fix defi coindix vaults. Risk param is disabled. Disable urllib3 warning bout not verified request url

* update tests for coindix - cassetes, csvs, txts

* update vaults docs

* Update _index.md

* run crypto tests with record_mode=rewrite

Co-authored-by: didierlopes.eth <[email protected]>
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.

3 participants