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

#155 Implement get_stock_earnings function. #249

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

t-tani
Copy link

@t-tani t-tani commented Dec 10, 2020

Hi @alvarobartt,
Thank you for creating an awesome scraping library.
I created the get_stock_eranings function for my own use.
I hope this pull request helps your development.

@t-tani t-tani changed the title #155 Implement get_stock_earnings funtion. #155 Implement get_stock_earnings function. Dec 10, 2020
@codecov
Copy link

codecov bot commented Dec 10, 2020

Codecov Report

Merging #249 (2e35aae) into master (ce02097) will decrease coverage by 0.07%.
The diff coverage is 89.13%.

❗ Current head 2e35aae differs from pull request most recent head d1cb63d. Consider uploading reports for the commit d1cb63d to get more accurate results

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
- Coverage   91.00%   90.93%   -0.08%     
==========================================
  Files          26       26              
  Lines        4949     4988      +39     
==========================================
+ Hits         4504     4536      +32     
- Misses        445      452       +7     

@alvarobartt alvarobartt self-assigned this Dec 22, 2020
@alvarobartt alvarobartt self-requested a review December 22, 2020 08:57
@alvarobartt alvarobartt added this to In progress in investpy v1.1.0 via automation Dec 22, 2020
Copy link
Owner

@alvarobartt alvarobartt left a comment

Choose a reason for hiding this comment

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

Everything seems fine besides the minor changes that I asked you to check, once you check those and the merge conflicts are solved I'll merge this as some users are requesting this feature and/or using your fork (which is no longer up to date as this PR is from Dec 2020).

Thanks for your work! 👍🏻

"User-Agent": random_user_agent(),
"X-Requested-With": "XMLHttpRequest",
"Accept": "text/html",
"Accept-Encoding": "gzip, deflate, br",
Copy link
Owner

Choose a reason for hiding this comment

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

br no longer accepted, you should update this line with "Accept-Encoding": "gzip, deflate" as reported in #328


objs = list()

if path_:
Copy link
Owner

Choose a reason for hiding this comment

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

Missing else statement, should raise an Exception just like all the other stock functions, please check https://github.com/alvarobartt/investpy/blob/master/investpy/stocks.py to see the latest changes and apply them here!

Copy link
Author

Choose a reason for hiding this comment

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

I think the else statement and Exception are already implemented as same as the following function.

def get_stock_dividends(stock, country):


def get_stock_earnings(stock, country):
"""
This function retrieves the earnings data from the specified stock. Earnings data include date of the
Copy link
Owner

Choose a reason for hiding this comment

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

Minor/Silly fix, but can you un-tab this so as to align it with all the docstrings? Thanks!

@@ -1506,3 +1506,176 @@ def search_stocks(by, value):
search_result.reset_index(drop=True, inplace=True)

return search_result


def get_stock_earnings(stock, country):
Copy link
Owner

Choose a reason for hiding this comment

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

As there are some changes in the stocks.py file in the master branch, could you pull the current file from the master and add your function at the end so that the PR has no merge conflicts?

@alvarobartt alvarobartt added the enhancement New feature or request label Apr 28, 2021
@alvarobartt
Copy link
Owner

Also, can you please check whether this functionality is the same as implemented in #242?

@markus080402
Copy link

Hi @t-tani , do you plan to create a new patch due to the new html format at investing.com and investpy?

Thanks and regards,
Markus

@t-tani
Copy link
Author

t-tani commented May 5, 2021

@alvarobartt Thank you for the code review. I will fix the code you pointed.
The difference between #242 and this pull request is that #242 retrieves data from earning calendar, and this pull request retrieves it from the stock page.

In #242, you could get the latest earnings data from all stocks; however, you could not specify the stock name. With this code, you could get the earnings from the specified stock.

@markus080402
Yes, I'll fix it for this pull request. Please give me a minute.

@t-tani
Copy link
Author

t-tani commented Jun 28, 2021

@alvarobartt
I'm sorry for the super late response. I merged the latest code and fix the code.
(Passing all the PyTests.)
Could you please check it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
investpy v1.1.0
In progress
Development

Successfully merging this pull request may close these issues.

investpy.get_stock_earnings() broken Financials Fetch EPS and forecast EPS etc
3 participants