Closed losgrandes closed 1 year ago
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.
Aside from the case sensitivity issue, this appears to work as well as I'd hoped when testing locally :)
@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.
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.
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.)
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.
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.)
https://github.com/mrhappyasthma/IsThisStockGood/issues/50#issuecomment-1328208406
I've changed the way PE ratios are fetched from MSN Money based on the findings in https://github.com/mrhappyasthma/IsThisStockGood/issues/57.