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 more specific error throwing based on PR 1918 #1928

Merged
merged 5 commits into from
May 11, 2024

Conversation

marcofognog
Copy link
Contributor

@marcofognog marcofognog commented May 10, 2024

Closes #270 and #1925

Purpose

It is useful to have the lib to throw specific errors for each failure, instead of the generic Exception.

With the error types being thrown, clients can chose to handle each failure scenario in different ways.

Ex:

    try:
        hist = ticker.history(period=period, interval=interval, raise_errors=True)
    except YFPricesMissingError as ex: 
            flash('The provided ticker symbol was not found, perhaps you mispelled it?')
    except YFInvalidPeriodError
            flash('Please provide a valid period')
            return redirect('/')

Backwards compatility

Because currently the exception being raise is the type Exception, we can safely assume no one is rescuing any of those new types. If any one is rescuing Exception code will continue to work in the same way.

Notes

This PR is basically the same as #1918 except I rebased to current dev and implemented changes suggested on the original PR.

I also tested it with my client app and it works as expected.

@paulmcq
Copy link

paulmcq commented May 10, 2024

Typo:
'''class YFTickerMissingError(YFException):
def init(self, ticker, rationale):
super().init(f"${ticker}: possibly delisted; {rationale}")
self.rationale = rationale
self.ticker = ticker ## rationale
'''

@marcofognog
Copy link
Contributor Author

Typo: '''class YFTickerMissingError(YFException): def init(self, ticker, rationale): super().init(f"${ticker}: possibly delisted; {rationale}") self.rationale = rationale self.ticker = ticker ## rationale '''

I'm sorry, I still can't see the typo, could you be more specific?

@paulmcq
Copy link

paulmcq commented May 10, 2024 via email

@marcofognog
Copy link
Contributor Author

At line 25 of yfinance/exceptions.py. the new code says self.ticker = rationale but surely that should be self.ticker = ticker because line 24 is self.rationale = rationale Sorry I was not more explicit.

Good catch! Thank you very much.

@marcofognog marcofognog marked this pull request as ready for review May 11, 2024 11:45
@marcofognog
Copy link
Contributor Author

FYI @elibroftw

Copy link
Collaborator

@ValueRaider ValueRaider left a comment

Choose a reason for hiding this comment

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

Final point: can you git-squash some commits to shorten history, at least your last few commits? Helps this https://github.com/ranaroussi/yfinance/network

Pipfile Outdated Show resolved Hide resolved
yfinance/version.py Outdated Show resolved Hide resolved
yfinance/ticker.py Outdated Show resolved Hide resolved
@marcofognog
Copy link
Contributor Author

Final point: can you git-squash some commits to shorten history, at least your last few commits? Helps this https://github.com/ranaroussi/yfinance/network

Sure. I'm squashing only mine to not loose the author on the previous commits.

@marcofognog marcofognog requested a review from ValueRaider May 11, 2024 17:43
@ValueRaider ValueRaider merged commit 070f135 into ranaroussi:dev May 11, 2024
1 check passed
@ValueRaider ValueRaider mentioned this pull request May 11, 2024
@marcofognog marcofognog changed the title Add more specific error thowring base on PR 1918 Add more specific error thorwing base on PR 1918 May 13, 2024
@marcofognog marcofognog changed the title Add more specific error thorwing base on PR 1918 Add more specific error throwing base on PR 1918 May 13, 2024
@marcofognog marcofognog changed the title Add more specific error throwing base on PR 1918 Add more specific error throwing based on PR 1918 May 13, 2024
@paulmcq
Copy link

paulmcq commented May 19, 2024

Line 25 of exceptions.py is still wrong:
#1928 (comment)

@ValueRaider
Copy link
Collaborator

self.ticker = ticker

Looks fine to me

@paulmcq
Copy link

paulmcq commented May 19, 2024

You are correct. I was apparently looking at the wrong version. Please excuse the spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants