fix(models): type SubscriptionEvent from the event the API sends (#149) - #150
Merged
Merged
Conversation
SubscriptionEvent declared type, code, payload and created_at, none of which GET /v1/subscriptions/events sends, so they read None on every real event, while id, observed_at, snapshot, deltas, source and tool_name were untyped pydantic extras. - Required id, seq, watch_id, observed_at (datetime), snapshot and deltas, matching null: false in the API's watch_events schema; optional source and tool_name (nullable columns). - snapshot -> Dict[str, SubscriptionEventSnapshot] (price, currency, optional change_24h_pct / as_of); deltas -> Dict[str, SubscriptionEventDelta] (price_change, optional pct_change, which the API omits for a zero prior price). Both exported. - type, code and payload removed: no event field corresponds to them. created_at kept as a deprecated property returning observed_at. - unwrap_events_page drops its seq-is-None guard; seq is now required. - Existing event fixtures updated to the live shape. Closes #149 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo
Resolve the CHANGELOG ### Fixed conflict with #148 by keeping both entries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
…cessors (#149) 1.16.0 is a minor release, so type, code and payload are not removed. They become @Property accessors that return None and emit DeprecationWarning naming the replacement (or that there is none), like created_at -> observed_at. They are not pydantic fields and are absent from model_dump(). All four are scheduled for removal in 2.0.0; CHANGELOG moves them under ### Deprecated. Tests: each accessor returns None/observed_at and warns on every access; none is serialized; parsing and polling all 223 live events (fixture captured 2026-09-13) emits no deprecation warning. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo
Merged
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
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
SubscriptionEventdeclaredtype,code,payloadandcreated_at.GET /v1/subscriptions/eventshas never sent any of them, so they readNoneon every real event. The fields the API does send (id,observed_at,snapshot,deltas,source,tool_name) were untyped pydantic extras, andobserved_atstayed a plain string.The model is now typed from the wire shape. That shape is
WatchEvent#as_poll_jsonon oilpriceapi-apiorigin/main, confirmed against 223 live events.idstr, requiredseqint, requirednull: falsewatch_idstr, requirednull: falseobserved_atdatetime(tz-aware), requirednull: false, validated presencesnapshotDict[str, SubscriptionEventSnapshot], requirednull: false, default{}deltasDict[str, SubscriptionEventDelta], requirednull: false, default{}.{}on a watch's first eventsourceOptional[str]tool_nameOptional[str]SubscriptionEventSnapshotcomes fromMarketBriefBuilder#snapshot_hash:price: floatandcurrency: strare always built.change_24h_pct: Optional[float]is nil when there is no 24h comparison.as_of: Optional[datetime]is built with a nil-safe call.SubscriptionEventDeltacomes fromWatch#compute_deltas:price_change: floatis always set.pct_change: Optional[float]is removed by.compactwhen the prior price is 0.deltasentirely when either snapshot lacks a price. The SDK does not fill it in.Both new models are exported from
oilpriceapi. Sync and async share the model.An event missing a required field raises
OilPriceAPIError(code="MALFORMED_RESPONSE")throughsubscriptions.events().unwrap_events_pagedrops itsseq is Noneguard, becauseseqis now required.No pydantic validator was added. The old
created_atfield_validatoris removed; pydantic parses the ISO timestamps natively.Deprecations (no attribute removed; removal in 2.0.0)
1.16.0 is a minor release, so no public attribute is removed. The four names the API never sends stop being pydantic fields and become plain
@propertyaccessors. Each emits aDeprecationWarningon every access, and none appears inmodel_dump()/model_dump_json().typeNone, as beforecodeNone, as beforelist(event.snapshot): an event covers every watched codepayloadNone, as beforeevent.snapshotandevent.deltascreated_atobserved_at; wasNoneobserved_atevent.type,event.codeandevent.payloadkeep returningNone, as they always did, so existing callers do not crash.created_atnow returns the real timestamp instead ofNone.Existing fixtures in
test_subscriptions_resource.py,test_async_subscriptions_resource.pyandtest_subscriptions_list_events_strict.pyused the invented{seq, watch_id, type, code}shape. They are updated to the live shape.Live evidence (api.oilpriceapi.com, 2026-09-13)
A full page-through of
GET /v1/subscriptions/events(read-only) returned 223 events, seq 1–223, one watch.idstr,seqint,watch_idstr,observed_atstr,snapshotdict,deltasdict,sourcestr,tool_namestr.as_ofstr,pricefloat,currencystr andchange_24h_pctfloat in 223 of 223.pct_changeandprice_changefloats in 222 of 222. Only seq 1 haddeltas: {}.All 223 raw events parsed with the new model:
This run used
-W error::DeprecationWarning, so parsing is also proven warning-free. The nullable inner fields were never null in this sample. They areOptionalbecause the API code allows nil, not because it was observed.Red (verbatim live events seq 1 and seq 223, run on unchanged
origin/mainc6f76ec)Failure reasons, counted:
The 3 that pass on main are guards: nullable attribution stays
None, a code absent fromdeltasis not filled in, and parsing emits no warning.Green
The 34 tests cover:
type,codeandpayloadreturnNone, andcreated_atreturnsobserved_at. Each warns once per access, including on a repeat access, and the warning names2.0.0and the replacement.model_dump()ormodel_dump_json().tests/unit/fixtures/subscription_events_live_2026-09-13.json) runs undersimplefilter("error"). Polling them through the real sync and async clients raises noDeprecationWarning.SubscriptionEventwarnings. Building a sync client without matplotlib emits an unrelatedImportWarning, filed as [P3][bug] Constructing OilPriceAPI emits ImportWarning when matplotlib is not installed #151.Full suite. The 3 failures are the live
tests/integration/test_demo_contract.py429s on both runs.origin/mainc6f76ec (measured on #147's tree)origin/main0266d08 (head 30048cd)ruff check oilpriceapi/plus the touched test files: All checks passed!mypy oilpriceapi/ --ignore-missing-imports, in a CI-equivalent.[dev]environment: Success, no issues found in 55 source files.python scripts/validate_storefront_claims.py: validated 76 public surfaces.Merge order
#148 is merged (
0266d08). This branch hasorigin/mainmerged in, with theCHANGELOG.md### Fixedconflict resolved by keeping both entries. No other open PR touches these files.Closes #149
🤖 Generated with Claude Code
https://claude.ai/code/session_015ao5paex73xXvuM424Libo