WIP: expand AsyncMlb endpoint coverage for v1.1.0 (#305) - #325
Open
Mattsface wants to merge 15 commits into
Open
WIP: expand AsyncMlb endpoint coverage for v1.1.0 (#305)#325Mattsface wants to merge 15 commits into
Mattsface wants to merge 15 commits into
Conversation
…ion and team roster/coaches Adds get_sport(s), get_league(s), get_division(s), get_team_roster, and get_team_coaches to AsyncMlb, per issue #305's plan to expand async coverage in small, reviewable batches. New shared parsers (_parsers/sports.py, leagues.py, divisions.py, roster.py) are reused by both Mlb and AsyncMlb; Mlb's existing methods are refactored internally onto them with no behavior change. AsyncMlb docstrings are mirrored from their Mlb counterparts. Adds parser tests, AsyncMlb endpoint tests, sync/async parity tests, and live external smoke tests for the new endpoints, and extends the frozen ASYNC_MLB_PUBLIC_METHOD_MANIFEST and docs/public-api.md accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtqT3jocrSJRouEew7Nnto
Continues issue #305's endpoint expansion. Adds a shared _parsers/seasons.py parser reused by both Mlb (refactored internally, no behavior change) and AsyncMlb, mirrors Mlb's docstrings onto the new AsyncMlb methods, and covers them with parser, endpoint, sync/async parity, and live external smoke tests. Renames the new tests/parsers/test_seasons.py to test_seasons_parser.py to avoid a pytest basename collision with the existing tests/external_tests/seasons/test_seasons.py (no test __init__.py packages exist yet, so basenames must be unique repo-wide); issue #322 tracks resolving this class of collision for good. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtqT3jocrSJRouEew7Nnto
…e to AsyncMlb Continues issue #305's endpoint expansion in two closely related groups that landed together: - get_venue/get_venues: new shared _parsers/venues.py, reused by Mlb (refactored internally, no behavior change) and AsyncMlb. get_venue faithfully preserves Mlb's documented quirk of returning [] (not None) on a 400-499 status while still falling through to None on an empty 200. Also fixes the shared assert_matches_sync test helper, which only handled scalar query params and silently mis-compared get_venue's list-valued hydrate param; it now correctly expands list values into repeated query pairs, matching how Requests/HTTPX actually serialize them. - get_standings/get_attendance: new shared _parsers/standings.py and _parsers/attendance.py. While porting get_attendance, found and fixed a real bug in Mlb: its "at least one of team_id/league_id/league_list_id" guard used any(required_args), which iterates dict keys (always truthy) instead of values, so the guard never fired and a bare get_attendance() call silently issued an unfiltered request. Fixed to any(required_args.values()) with a regression test proving the guard now short-circuits, and both clients port the corrected behavior identically. Each endpoint gets parser, AsyncMlb endpoint, sync/async parity, and live external smoke test coverage, plus the frozen ASYNC_MLB_PUBLIC_METHOD_MANIFEST and docs/public-api.md are extended accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtqT3jocrSJRouEew7Nnto
Continues issue #305's endpoint expansion. Adds shared _parsers/draft.py and _parsers/awards.py, reused by Mlb (refactored internally, no behavior change) and AsyncMlb. get_awards preserves Mlb's endpoint string with a trailing "?" (awards/{id}/recipients?); both Requests and HTTPX treat it as a harmless empty query separator, so it's kept as-is for parity rather than "fixed". That trailing "?" exposed a gap in the shared assert_matches_sync test helper: it compared request.url.path against the raw endpoint string, but neither client's parsed path retains a trailing "?". Fixed with a .rstrip("?") normalization. Covers both endpoints with parser, AsyncMlb endpoint, sync/async parity, and live external smoke tests, and extends the frozen ASYNC_MLB_PUBLIC_METHOD_MANIFEST and docs/public-api.md accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtqT3jocrSJRouEew7Nnto
Continues issue #305's endpoint expansion. Fixes a bug in Mlb.get_homerun_derby found while porting it: the 400-499 guard was a bare `None` expression instead of `return None`, so a 4xx response body containing a truthy "status" key would fall through into HomeRunDerby(**data) and raise ValidationError instead of returning None. Fixed to `return None`, following the same fix-then-port approach used for get_attendance. Adds a shared _parsers/homerunderby.py reused by both Mlb (refactored internally) and AsyncMlb, plus parser, endpoint, sync/async parity (including a dedicated regression test for the malformed-error-body case), and live external smoke test coverage. Extends the frozen ASYNC_MLB_PUBLIC_METHOD_MANIFEST and docs/public-api.md accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtqT3jocrSJRouEew7Nnto
Continues issue #305's endpoint expansion. Adds get_team_id, get_people_id, get_sport_id, get_league_id, get_division_id, and get_venue_id to AsyncMlb. Extracts the identical filter logic these six Mlb methods each duplicated (case-insensitive search_key match, collect id, skip KeyError) into a single shared helper, _helpers/id_lookup.py::find_ids_by_key, and refactors Mlb onto it (no behavior change, verified against the live API). Each AsyncMlb method mirrors its sync counterpart's docstring and preserves its individual quirks (which methods set a trimming `fields` param, and that get_venue_id does not set `hydrate` the way get_venue/get_venues do). Adds a new tests/helpers/ directory (mirroring tests/parsers/) for the shared helper's unit tests, plus endpoint, sync/async parity, and live external smoke test coverage for all six methods, and extends the frozen ASYNC_MLB_PUBLIC_METHOD_MANIFEST and docs/public-api.md accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtqT3jocrSJRouEew7Nnto
…AsyncMlb Continues issue #305's endpoint expansion with its largest batch: the game group (get_game, get_game_play_by_play, get_game_line_score, get_game_box_score, get_game_ids). get_game uses the v1.1 live feed endpoint, which AsyncMlb previously had no support for. AsyncMlb now constructs a second AsyncMlbDataAdapter for v1.1 that shares one HTTPX client with the v1 adapter, mirroring Mlb's shared-Session pattern: the v1 adapter resolves and owns the client (library-created when the caller passes none), and v1.1 borrows it without ever closing it directly. Sharing the client this way exposed a retry-budget bug in AsyncMlbDataAdapter: retry eligibility was tied 1:1 to "does this adapter own its client", so a naive v1.1 adapter borrowing v1's client would never retry, even when the underlying client is library-owned. Mlb doesn't have this problem because its retry policy is mounted once on the shared Session, not per adapter version. Fixed by adding a retries_enabled parameter to AsyncMlbDataAdapter, decoupling close-ownership from retry eligibility, defaulting to prior behavior for standalone use. Verified directly and with dedicated regression tests that both adapters now retry exactly when the shared client is library-owned, and neither retries with a caller-injected client. Adds a shared _parsers/games.py reused by both Mlb (refactored internally) and AsyncMlb, faithfully preserving get_game_line_score's documented missing 400-499 guard. Also generalizes the shared sync_request_for/assert_matches_sync test helpers, which only knew about the v1 adapter and hardcoded the /api/v1/ prefix, to check both adapters and use whichever one actually fired. Full parser, endpoint, sync/async parity, and live external smoke test coverage for all five endpoints, plus the frozen ASYNC_MLB_PUBLIC_METHOD_MANIFEST and docs/public-api.md (including a new "API versions used by AsyncMlb" section paralleling Mlb's) are extended accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtqT3jocrSJRouEew7Nnto
…tructor param with a private setter Follow-up to the v1.1 adapter sharing added for get_game (issue #305). The previous fix added retries_enabled: bool | None to AsyncMlbDataAdapter.__init__ so AsyncMlb could tell its borrowed v1.1 adapter that the shared client is library-owned, even though v1.1 itself received a non-None client and would otherwise conclude it's using a caller-injected client and disable retries. That parameter looked like public, supported configuration on a documented class's constructor, when it was really an internal coordination detail: only AsyncMlb, which actually owns the shared transport, has the information to make that call, and no standalone AsyncMlbDataAdapter caller should reach for it. Removes the constructor parameter entirely (its public signature is back to exactly what it was before the v1.1 work) and replaces it with a private _set_retries_enabled() method, explicitly documented as an internal coordination hook rather than public API. AsyncMlb.__init__ now calls that method instead of reassigning the private _retries_enabled attribute directly, so the coordination point is discoverable and testable in isolation rather than an undeclared attribute poke. Adds two unit tests directly on the new setter: one proving it only affects retry eligibility and never grants close-ownership over a client the adapter didn't create, and one proving the override changes actual retry behavior (a scripted transient failure is retried), not just a stored flag. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BtqT3jocrSJRouEew7Nnto
…sport
AsyncMlb's ownership pointed the wrong way: __init__ built the v1 adapter
with client=client, let that adapter create the shared httpx.AsyncClient, then
reached back into self._mlb_adapter_v1._client to construct the v1.1 adapter.
aclose() delegated the parent's shutdown to one of its children, with the
other child required not to close the thing it shares. Underneath that,
AsyncMlbDataAdapter._owns_client answered two different questions at once
("who closes this client" and "am I allowed to retry"), which is why
_set_retries_enabled() existed at all: to undo the wrong conclusion the v1.1
adapter reached about retry eligibility after receiving a non-None client.
Mlb does not have this problem because it does not implement retries -- it
configures them, once, by mounting an HTTPAdapter carrying the retry policy
onto the Session it creates. HTTPX has the same extension seam:
AsyncClient(transport=...) accepts any AsyncBaseTransport, the position
HTTPAdapter occupies in Requests. Moving the retry loop there makes retries a
property of the client, so two adapters sharing one client share one policy
by construction and cannot disagree about it.
Adds a private mlbstatsapi/_async_transport.py:
- MlbAsyncRetryTransport wraps an inner transport (default
httpx.AsyncHTTPTransport()) with the existing create_retry_policy() budget
and backoff accounting, moved verbatim from
AsyncMlbDataAdapter._request_with_retries /_sleep_before_retry. On an
exhausted budget it re-raises the underlying httpx exception rather than
translating it -- transports are contractually expected to raise httpx
errors, translation stays with the adapter.
- create_library_async_client() is the async counterpart of
_configure_library_session(): builds a client with the package User-Agent
and MlbAsyncRetryTransport() mounted.
AsyncMlb.__init__ now owns the client exactly as Mlb.__init__ owns the
Session (self._owns_client, self._client, self._closed, created via
create_library_async_client() when the caller passes none) and hands that one
client to both adapters. aclose() closes self._client directly instead of
delegating to the v1 adapter.
AsyncMlbDataAdapter's public constructor is unchanged. get() now calls
self._client.get() directly, wrapped in the same two except clauses the sync
adapter's error path mirrors (TimeoutException -> MlbTimeoutError,
RequestError -> MlbTransportError; ConnectTimeout/ConnectError fall into the
right one because ConnectTimeout is a TimeoutException subclass). Deleted
_set_retries_enabled, _retries_enabled, _retry_policy,
_request_with_retries, and _sleep_before_retry.
Retargets the test seam from patching httpx.AsyncClient to patching
httpx.AsyncHTTPTransport inside _async_transport, so tests exercise the real
client, the real retry transport, and the real headers with a MockTransport
at the bottom. _owned_adapter/_injected_adapter keep their names; a new
_retry_policy_of() accessor reads the policy from the client's transport for
tests that mutate it. Replaces the two _set_retries_enabled tests with three
that assert the new shape: a library-created client mounts
MlbAsyncRetryTransport, an injected client's transport is left exactly as
supplied, and mounting MlbAsyncRetryTransport on an injected client makes it
retry (the caller-facing opt-in). In test_async_mlb.py, ownership assertions
move from the adapters to AsyncMlb itself, and the cleanup-failure test mocks
mlb.aclose rather than an adapter's.
Testing:
- poetry run pytest tests/ --ignore=tests/external_tests: 973 passed before
this change (checked out from the unmodified branch tip), 974 after (net
+1: two _set_retries_enabled tests removed, three new transport-ownership
tests added).
- tests/test_sync_async_parity.py: 96 passed, unchanged -- AsyncMlb's
observable behavior did not move.
- tests/external_tests/async_mlb/: 26 passed against the live API.
Risk: internal-only change to a private transport layer behind AsyncMlb's and
AsyncMlbDataAdapter's unchanged public constructors. Retry budgets, backoff
timing, exception mapping, 404/strict_http behavior, the User-Agent, and
caller-injected-client ownership rules are all covered by tests and were not
changed on purpose.
Intentionally left out: MlbAsyncRetryTransport stays private. Exporting it
would turn "an injected client gets no library retries" from a limitation
into a documented opt-in, which is a public-API decision -- see the PR
description.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BtqT3jocrSJRouEew7Nnto
Mattsface
changed the title
Member
Author
|
@claude take a look at this WIP |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
Ports the last four stats methods to AsyncMlb at strict parity with Mlb:
get_stats, get_player_stats, get_team_stats, and get_players_stats_for_game.
All four sync methods ended in the same copy-pasted tail -- short-circuit on
400-499, then create_split_data(data['stats']) if present and truthy, else {}.
That block existed four times in mlb_api.py. It moves to a shared
_parsers/stats.py::parse_split_stats(), following the pattern the rest of the
async port already uses, and both clients now call the one copy.
Also fixes a real bug on the sync side while collapsing those copies:
get_players_stats_for_game accepted **params and never passed ep_params to the
adapter, so every caller-supplied keyword was silently discarded before the
request was built. Both clients now forward them, covered by a named
regression test in the parity suite.
No new types, constants, or validation: an unrecognized stat type or group
still yields {} rather than raising, matching sync exactly. docs/public-api.md
notes that sharp edge alongside the newly supported methods.
Docstring corrections on Mlb.get_players_stats_for_game: it described game_id
as "list of stat types", person_id as "the team id", and its example called
get_player_stats_for_game, which is not a method.
Tests: 1016 passed (up from 974). tests/external_tests/stats/ 30 passed
against the live API, confirming the sync refactor did not move behavior.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ports get_persons, get_scheduled_games_by_date, and get_gamepace. AsyncMlb now
exposes every endpoint method Mlb does; the only remaining public difference is
that close() is spelled aclose().
Two more shared parsers, following the established pattern:
_parsers/schedules.py gains parse_scheduled_games(), and _parsers/gamepace.py
is new. Both replace inline loops/conditionals in mlb_api.py, so the two
clients share one copy.
Fixes an httpx/requests divergence that would have silently broken get_gamepace
on the async side. Mlb builds that request as endpoint="gamePace?season=2021"
with ep_params={"sportId": 1} and relies on Requests merging the endpoint's
query with the params. HTTPX does not merge -- passing params replaces a query
already on the URL -- so copying the sync idiom drops the season entirely and
silently returns whatever the unfiltered endpoint gives back. AsyncMlb passes
the season as an ordinary param instead, which produces a byte-identical
request. Verified against the live API: async and sync return equal GamePace
objects for season 2021.
tests/test_async_mlb.py's assert_matches_sync() had the same blind spot -- it
compared url.path against the raw endpoint string and would not have caught
this. It now splits an endpoint's embedded query and folds it into the expected
params, which is what Requests does, so the expectation is the merged query
either client must end up sending. This also subsumes the get_awards trailing-?
special case it previously carried.
get_scheduled_games_by_date preserves Mlb's quirk of returning None rather than
the [] its annotation promises when no date selector was given, asserted
explicitly in the parity suite rather than left implicit.
Tests: 1057 passed (up from 1016). tests/external_tests/ 148 passed, 1 skipped
against the live API.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
@claude please do a pass |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
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
1 participant
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.
Why
Continue the v1.1.0 async work by expanding
AsyncMlbbeyond the initial endpoint set.The goal is to make async support useful across more of the existing
MlbAPI while keeping sync behavior unchanged and maintaining the same models, parsing, HTTP behavior, and error semantics where an async endpoint is supported.Tracks #305.
What
AsyncMlbcoverage across teams, people, sports, leagues, divisions, seasons, venues, rosters, standings, attendance, draft, awards, Home Run Derby, and game endpointsThis is still WIP. Follow-up transport work is tracked in #324.
Tests
Added and expanded deterministic tests for:
AsyncMlbendpoint behaviorLive async smoke tests were also expanded for supported MLB API paths.
The full branch should still go through normal CI and final live validation before merge.
Risk and impact
Risk: Normal
The async API is additive, and the existing synchronous public API is intended to remain unchanged. However, this PR also extracts parsing logic shared by the sync and async clients and changes async transport internals, so regressions could affect more than one endpoint.
If something goes wrong, the most likely impact would be incorrect parsing, request parameters, lifecycle/retry behavior, or a sync/async behavior mismatch. A regression in the shared parsers could also affect existing synchronous calls.
The parity, parser, transport, and live smoke tests are intended to catch those cases before release.