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

New Open Source menu #1603

Merged
merged 24 commits into from
Apr 4, 2022
Merged

New Open Source menu #1603

merged 24 commits into from
Apr 4, 2022

Conversation

jose-donato
Copy link
Contributor

Description

This PR adds a new menu to alternative context. This new menu aims to cover open source data and uses Github api as source.
It contains three different commands:

  • rs: displays a certain repository summary

image

  • sh: displays star history of a certain repository overtime

image

  • tr: displays top repos sorted by stars or forks and can be filtered by categories (e.g., finance)

image

Three features to start but github api is nice and has lot of data. This menu will help users analyse different repos and other open source data.

How has this been tested?

Tested controller and view/model.

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 M Medium T-Shirt size feature label Mar 31, 2022
Copy link
Contributor

@piiq piiq left a comment

Choose a reason for hiding this comment

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

We need proper api key handling here
image

@jose-donato
Copy link
Contributor Author

We need proper api key handling here image

thanks. will take a look

@jose-donato
Copy link
Contributor Author

@piiq should be good now

@piiq
Copy link
Contributor

piiq commented Apr 1, 2022

@jose-donato not so fast
image
lol

@piiq
Copy link
Contributor

piiq commented Apr 1, 2022

@jose-donato can you add an .openbb script for integration tests?

in addition to this I would like to propose renaming the os command to oss or gh or github because os is a built in python package and we might have some confusion when using the functionality of the menu from python or jupyter

@jose-donato
Copy link
Contributor Author

@jose-donato can you add an .openbb script for integration tests?

in addition to this I would like to propose renaming the os command to oss or gh or github because os is a built in python package and we might have some confusion when using the functionality of the menu from python or jupyter

renamed to oss!

the third time is the charm (hopefully)

@jose-donato
Copy link
Contributor Author

@DidierRLopes @Chavithra it only happened after pulling branches from main, do you know what is wrong here?

@piiq
Copy link
Contributor

piiq commented Apr 2, 2022

@jose-donato just tested it. works great, although it took VERY long to generate redis' star history
image

possible to add github to the keys menu?

@jose-donato
Copy link
Contributor Author

@jose-donato just tested it. works great, although it took VERY long to generate redis' star history image

possible to add github to the keys menu?

yes it takes a lot of time. not sure we can optimize it though. only if we do the requests in parallel! i researched a little bit and it looks like we can only get the star history this way.

yea i forgot about that! will add

Copy link
Contributor

@piiq piiq left a comment

Choose a reason for hiding this comment

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

This is good for me
@Chavithra can you have a look if logging is implemented correctly here?

@Chavithra Chavithra added the do not merge Label to prevent pull request merge label Apr 4, 2022
@Chavithra
Copy link
Contributor

Chavithra commented Apr 4, 2022

I think we need to filter the user agent like in this example :
https://github.com/OpenBB-finance/OpenBBTerminal/tree/main/tests#38-known-issue--solution

@Chavithra Chavithra self-requested a review April 4, 2022 14:43
@Chavithra Chavithra removed the do not merge Label to prevent pull request merge label Apr 4, 2022
Copy link
Contributor

@Chavithra Chavithra left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Thanks José !

@piiq piiq merged commit 01391c2 into main Apr 4, 2022
@piiq piiq deleted the os branch April 4, 2022 16:08
mrriteshranjan added a commit to mrriteshranjan/OpenBBTerminal that referenced this pull request Apr 6, 2022
mrriteshranjan added a commit to mrriteshranjan/OpenBBTerminal that referenced this pull request Apr 7, 2022
mrriteshranjan added a commit to mrriteshranjan/OpenBBTerminal that referenced this pull request Apr 12, 2022
mrriteshranjan added a commit to mrriteshranjan/OpenBBTerminal that referenced this pull request Apr 12, 2022
mrriteshranjan added a commit to mrriteshranjan/OpenBBTerminal that referenced this pull request Apr 12, 2022
mrriteshranjan added a commit to mrriteshranjan/OpenBBTerminal that referenced this pull request Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat M Medium T-Shirt size feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants