feat: add Yahoo Finance investment price provider - #464
Open
WMP wants to merge 4 commits into
Open
Conversation
Adds a new price provider that fetches daily closing prices from the unofficial Yahoo Finance chart API (v8/finance/chart). No API key is required, making it a zero-configuration option for stocks and ETFs listed on major exchanges. - Supports standard refill (2y) and incremental (5d) fetch modes - Extracts chart.error.description from error responses for clear messages - Rate limit policy: 10 req/min, 1000 req/day - Symbols follow Yahoo Finance conventions, e.g. AGGG.L (LSE), ISAC.MI (Borsa Italiana) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds YahooFinanceProvider with HTTP fetch/parsing/error mapping, registers it in the InvestmentPriceProviderRegistry, and updates the API test to include the new provider. ChangesYahoo Finance Provider
Sequence Diagram(s)sequenceDiagram
participant App as App / Service
participant Registry as InvestmentPriceProviderRegistry
participant Provider as YahooFinanceProvider
participant Http as GuzzleHttp_Client
participant Yahoo as Yahoo_Finance_API
App->>Registry: Resolve provider by key (yahoo_finance)
Registry->>Provider: Return instance
App->>Provider: fetchPrices(investment, from?, refill?)
Provider->>Http: GET /v8/finance/chart/{symbol}?range=...
Http->>Yahoo: HTTP request
Yahoo-->>Http: JSON response
Http-->>Provider: Response body
Provider->>Provider: Parse JSON, validate timestamps & close arrays
alt valid data
Provider-->>App: [{date, price}, ...]
else invalid / error
Provider-->>App: throws InvalidPriceDataException or PriceProviderException
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/Services/InvestmentPriceProviders/YahooFinanceProvider.php (1)
128-131: Move the no-op explanation out of the method body.The empty implementation is fine, but the inline comment does not need to live inside executable code. A short PHPDoc is cleaner if you want to keep the rationale.
💡 Suggested refactor
- public function validateCredentials(array $credentials): void - { - // No credentials needed - } + /** + * Yahoo Finance does not require user credentials. + * + * `@param` array<string, mixed> $credentials + */ + public function validateCredentials(array $credentials): void + { + }As per coding guidelines, "Use PHPDoc blocks instead of inline comments, and avoid comments within code unless the logic is exceptionally complex."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/InvestmentPriceProviders/YahooFinanceProvider.php` around lines 128 - 131, The method validateCredentials(array $credentials): void in YahooFinanceProvider currently contains an inline comment explaining it's a no-op; move that rationale into a PHPDoc block above the method (e.g., /** No credentials required for YahooFinanceProvider. */) and remove the inline comment from the method body so the implementation stays empty but documented; update the PHPDoc to reference the method name validateCredentials to clarify why it intentionally does nothing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Services/InvestmentPriceProviders/YahooFinanceProvider.php`:
- Around line 23-24: The provider sets $range = $refill ? '2y' : '5d' but later
(in the loop around the logic that falls back to now()->subDays(5)) still clamps
the start to 5 days and compares quote timestamps at midnight, causing two-year
refills to be trimmed and mismatches when callers pass Carbon with time
components; update the logic in YahooFinanceProvider (the $range assignment and
the code that computes $start/$from and the loop that filters quotes) to compute
the request window as the max span needed (e.g., if $from is provided and older
than 5 days use a longer range like '2y' or compute a dynamic range covering
$from), compare dates using startOfDay() or normalize both sides to date-only
before filtering, and ensure the filtering loop uses inclusive date comparisons
so quotes from the requested $from are retained.
- Around line 99-110: The catch block for ClientException currently always
throws InvalidPriceDataException; change it to inspect
$e->getResponse()->getStatusCode() (from the ClientException $e) and for 429 and
403 (and any other transient 4xx you deem retriable) throw a
PriceProviderException instead, while keeping 400 and 404 mapped to
InvalidPriceDataException; preserve the existing attempt to parse $description
from the response body and pass that (or the default message) into the thrown
exception, referencing the same InvalidPriceDataException and
PriceProviderException classes and the $investment->symbol identifier.
---
Nitpick comments:
In `@app/Services/InvestmentPriceProviders/YahooFinanceProvider.php`:
- Around line 128-131: The method validateCredentials(array $credentials): void
in YahooFinanceProvider currently contains an inline comment explaining it's a
no-op; move that rationale into a PHPDoc block above the method (e.g., /** No
credentials required for YahooFinanceProvider. */) and remove the inline comment
from the method body so the implementation stays empty but documented; update
the PHPDoc to reference the method name validateCredentials to clarify why it
intentionally does nothing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a08dad5-a34c-4232-bb74-6665e9fbd4a3
📒 Files selected for processing (2)
app/Providers/InvestmentPriceProviderServiceProvider.phpapp/Services/InvestmentPriceProviders/YahooFinanceProvider.php
- rawurlencode() symbol in URL path (handles ^INDEX-style tickers) - Compute $cutoff once before the loop using startOfDay() to avoid repeated Carbon parsing and correctly handle $from values with time components; $cutoff is null for refill so all 2y data is accepted - Distinguish transient (403/429) vs permanent (400/404) HTTP errors: transient errors throw PriceProviderException so the caller can retry, permanent errors throw InvalidPriceDataException - Move inline comment on validateCredentials() to PHPDoc block - Wrap getDisplayName/getDescription/getInstructions strings in __() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Yahoo Finance requires no credentials so it appears as available by default, alongside web_scraping. Updated the assertion count from 2 to 3 and added explicit checks for the yahoo_finance provider. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Services/InvestmentPriceProviders/YahooFinanceProvider.php (1)
25-26: Consider consolidating inline comments into method PHPDoc.The inline comments here and at lines 115-116 explain important business logic. Per coding guidelines, PHPDoc blocks are preferred over inline comments. These could be moved to a method-level
@noteor expanded@paramdocumentation describing the cutoff behavior and error classification strategy.Example PHPDoc consolidation
+ /** + * Fetch historical prices from Yahoo Finance. + * + * For incremental fetches ($refill=false), uses a 5-day range and filters + * prices to those on or after $from (defaulting to 5 days ago). + * For refill fetches ($refill=true), uses a 2-year range with no cutoff. + * + * `@throws` InvalidPriceDataException For invalid symbols (400/404) + * `@throws` PriceProviderException For transient errors (403/429) or network failures + */ public function fetchPrices(Investment $investment, ?Carbon $from = null, bool $refill = false): array { $range = $refill ? '2y' : '5d'; - // Default cutoff: beginning of today minus 5 days for incremental fetches. - // For refill, keep $from as null so all returned data is accepted. $cutoff = $fromAs per coding guidelines: "Prefer PHPDoc blocks over inline comments."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/InvestmentPriceProviders/YahooFinanceProvider.php` around lines 25 - 26, Move the important inline comments about the default cutoff and refill behavior and the error-classification strategy into the method-level PHPDoc for the relevant method in YahooFinanceProvider (e.g., the method that computes/uses $from for incremental vs refill fetches and handles API errors), replacing the inline comments at lines ~25-26 and ~115-116; add an `@note` (or expand `@param/`@return) that documents the "beginning of today minus 5 days" default cutoff, that $from is kept null for refill so all returned data is accepted, and the error classification rules, then remove the duplicate inline comments from the method body. Ensure the PHPDoc references the $from parameter and any error enums or helper methods used for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/Services/InvestmentPriceProviders/YahooFinanceProvider.php`:
- Around line 25-26: Move the important inline comments about the default cutoff
and refill behavior and the error-classification strategy into the method-level
PHPDoc for the relevant method in YahooFinanceProvider (e.g., the method that
computes/uses $from for incremental vs refill fetches and handles API errors),
replacing the inline comments at lines ~25-26 and ~115-116; add an `@note` (or
expand `@param/`@return) that documents the "beginning of today minus 5 days"
default cutoff, that $from is kept null for refill so all returned data is
accepted, and the error classification rules, then remove the duplicate inline
comments from the method body. Ensure the PHPDoc references the $from parameter
and any error enums or helper methods used for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a548f154-4b83-4f4f-9bca-41d8093c44f7
📒 Files selected for processing (2)
app/Services/InvestmentPriceProviders/YahooFinanceProvider.phptests/Feature/API/V1/InvestmentPriceProviderApiV1Test.php
Pint's global_namespace_import requires global-namespace classes to be imported via `use`. The catch block used \Throwable inline; replaced with imported Throwable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Services/InvestmentPriceProviders/YahooFinanceProvider.php (1)
149-154: 💤 Low valueConsider suppressing the PHPMD warning for the unused parameter.
The
$credentialsparameter is required by the interface but intentionally unused here. Adding a suppression annotation documents this intent and silences the static analysis warning.💡 Optional: Add suppression annotation
/** * No credentials are required for Yahoo Finance. + * + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ public function validateCredentials(array $credentials): void { }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Services/InvestmentPriceProviders/YahooFinanceProvider.php` around lines 149 - 154, The validateCredentials(array $credentials): void method in YahooFinanceProvider intentionally does not use the $credentials parameter; add a PHPMD suppression annotation to its docblock (e.g. `@SuppressWarnings`(PHPMD.UnusedFormalParameter)) to document intent and silence the static analysis warning while keeping the method signature required by the interface.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/Services/InvestmentPriceProviders/YahooFinanceProvider.php`:
- Around line 149-154: The validateCredentials(array $credentials): void method
in YahooFinanceProvider intentionally does not use the $credentials parameter;
add a PHPMD suppression annotation to its docblock (e.g.
`@SuppressWarnings`(PHPMD.UnusedFormalParameter)) to document intent and silence
the static analysis warning while keeping the method signature required by the
interface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68387f25-2e13-4ba8-8b2d-49c0f19e795c
📒 Files selected for processing (1)
app/Services/InvestmentPriceProviders/YahooFinanceProvider.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a new price provider that fetches daily closing prices from the Yahoo Finance chart API. No API key is required, making it a zero-configuration option for stocks and ETFs listed on major world exchanges.
chart.error.descriptionfrom 4xx error responses for clear, human-readable error messagesAGGG.L(London Stock Exchange),ISAC.MI(Borsa Italiana)Changes
app/Services/InvestmentPriceProviders/YahooFinanceProvider.php— new provider classapp/Providers/InvestmentPriceProviderServiceProvider.php— registeryahoo_financein the provider registryTest plan
AGGG.Lphp artisan investments:fetch-prices— verify prices are saved to the databaseINVALID.XX) — verify a clear error message is shownSummary by CodeRabbit
New Features
Chores