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

Fixed getting PE ratios from MSN #60

Merged
merged 3 commits into from
Nov 26, 2022

Conversation

losgrandes
Copy link
Contributor

I've changed the way PE ratios are fetched from MSN Money based on the findings in #57.

src/MSNMoney.py Outdated Show resolved Hide resolved
src/MSNMoney.py Outdated Show resolved Hide resolved
src/MSNMoney.py Outdated Show resolved Hide resolved
@mrhappyasthma
Copy link
Owner

Generally the code looks good. Thanks again for doing this.

I have to patch it locally to test it still, but looks sound to me. Left some comments, but mostly small stuff.

src/MSNMoney.py Outdated Show resolved Hide resolved
@mrhappyasthma
Copy link
Owner

Aside from the case sensitivity issue, this appears to work as well as I'd hoped when testing locally :)

Fault-tolerant PE Ratios extraction
Added tests for MSNMoney's _parse_pe_ratios
@losgrandes
Copy link
Contributor Author

losgrandes commented Nov 26, 2022

@mrhappyasthma All your suggestions have been applied. I've added unit tests for _parse_pe_ratios.

However, something is off, cause I get very different results for e.g. GOOG within several minutes. Seems like something random.

Now I'm geting for GOOG:

margin_of_safety_price: 26.6402
         sticker_price: 53.2804
         current_price: 97.6

Several minutes ago:

margin_of_safety_price: 391.2
         sticker_price: 782.4
         current_price: 97.6

I don't think there's anything wrong with PE ratios. I'll try to investigate this. Without knowing why this PR cannot be merged.

@losgrandes
Copy link
Contributor Author

I don't think there's anything wrong with PE ratios. I'll try to investigate this. Without knowing why this PR cannot be merged.

I reproduced this, but only when using Rule#1 Stock Screener. Let me look at what's off there.

@mrhappyasthma
Copy link
Owner

Could you clarify the repro steps for the issue you noticed with GOOG?

Were you just using the site and querying GOOG and seeing different results at different times?

(I'd be a bit surprised as I imagine most of the data should be pretty static. But perhaps there's either a bug somewhere or one of the data sources that we scrape is unreliable.)

@mrhappyasthma
Copy link
Owner

That said, I think the code here is sound. So I'm gonna merge it in. And if we can observe a reliable way to reproduce, we can debug further.

@losgrandes
Copy link
Contributor Author

Could you clarify the repro steps for the issue you noticed with GOOG?

Were you just using the site and querying GOOG and seeing different results at different times?

(I'd be a bit surprised as I imagine most of the data should be pretty static. But perhaps there's either a bug somewhere or one of the data sources that we scrape is unreliable.)

#50 (comment)

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.

None yet

2 participants