Add initial AsyncMlb client - #319
Merged
Merged
Conversation
- Expose AsyncMlb lazily from the package root - Add async team, people, and schedule endpoint methods - Support async context management and resource cleanup - Preserve original exceptions and cancellations during cleanup - Add offline lifecycle tests and live API endpoint coverage
- Move schedule request parameter building into a shared parser helper - Reuse the helper in AsyncMlb.get_schedule - Preserve existing handling for dates, game IDs, teams, and sports - Rename the live async test module to identify it as an external test
- Include lazy async client exports in package introspection - Distinguish missing schedule parameters from valid parameter dictionaries
The package root carried a stray closing paren after __dir__, which made
`import mlbstatsapi` a SyntaxError and took down all 17 offline test
modules at collection, not just the async ones. The lazy AsyncMlb and
AsyncMlbDataAdapter exports are unchanged.
Two endpoints on the vertical slice had drifted from the synchronous
client they port:
* get_teams took no sport_id at all, so it never sent the sportId query
parameter that Mlb.get_teams always sends.
* get_people was really get_persons: it posted personIds to `people`
rather than reading `sports/{sport_id}/players`. Its annotations also
referenced Union and List, neither imported, which stayed latent only
because annotations are deferred in this module.
Both now match Mlb in argument names, defaults, endpoint, parameter
construction, return type, and empty-result behavior. get_schedule and
the shared build_schedule_params helper are unchanged; they already
reproduce the sync logic exactly.
The async surface no longer offers a personIds lookup. Adding a
get_persons port is deliberately left out of this slice.
Tests:
* tests/test_async_mlb.py grows from 7 tests to 48. Endpoint tests drive
the real adapter over httpx.MockTransport instead of mocking the
adapter away, so a method that stopped issuing HTTP would fail rather
than pass against a mock. Adds the package-root import, client
ownership, idempotent cleanup, per-endpoint request and result
behavior, a parametrized parity check against Mlb, a signature-drift
guard, concurrency on a shared client, and absence of hidden fan-out
or background tasks. The transport-contract matrix stays in #302.
* The module now guards its import with pytest.importorskip("httpx"), so
a sync-only install skips it instead of erroring at collection.
* test_async_get_teams defined a scenario but never ran it. It now
executes, and it and test_async_get_people assert the result is
non-empty, since `all()` over an empty list proves nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013La4LovbUtWoZSrD3iQQKu
The suite had grown to 48 tests over 838 lines, well past what the #303 vertical slice is worth. It is now 20 tests over 443 lines, covering the public import, the lifecycle contract, per-endpoint request construction and parsed results, signature parity, and one concurrency case. What went, and why: * Exhaustive schedule argument combinations collapse to one representative date-range-with-team case plus the no-selector case. * Six empty-result permutations become two parametrized tests over the two ways an endpoint comes back with nothing: 404 and an empty 200. * The twelve-case request-construction matrix is gone. Each endpoint test now derives its expected endpoint and params from Mlb itself through assert_matches_sync(), so drift is still caught where the endpoint is asserted rather than in a separate matrix. * The asyncio.all_tasks() tests asserted an implementation detail and are dropped. The standalone fan-out test is dropped too: _Handler asserts the request count when a test reads .request, so every endpoint test rules out fan-out on its own. * Status mapping and transport behavior belong to the adapter suite and the #302 matrix; payload parsing belongs to tests/parsers/. Neither is re-asserted here. Cleanup is also correct now, which it was not before. Tests built a bare AsyncMlb, which eagerly opens an HTTPX client, then replaced the adapter's aclose with a mock, so the client was never closed; the sync parity test leaked a requests Session per case. Eighteen tests leaked. Rather than a global registry, teardown is a local async_mlb() context manager that closes the adapter's client in its finally. It closes the client directly instead of calling AsyncMlb.aclose(), because the one test that mocks aclose would otherwise still leak. Verified with an instrumented run that counts unclosed clients and sessions: eighteen leaking tests before, zero after. Seven mutations of the client -- dropped sportId, get_people reverting to the people endpoint, a lost schedule short-circuit, a no-op aclose, aclose closing an injected client, a swallowed cleanup failure, and a drifted default -- all still fail, so the smaller suite protects what the larger one did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013La4LovbUtWoZSrD3iQQKu
Member
Author
|
@claude please do a pass |
|
Claude finished @Mattsface's task in 2m 53s —— View job Claude finished the review passTasks
SummaryOverall this is a well-scoped, clean vertical slice. Findings1. This repo treats
2. Minor style nits in
3. Docstring inconsistency — low Verified as correct
Note on test executionThis sandbox's Bash tool required interactive approval for |
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.
Why
This is the first public
AsyncMlbimplementation for the 1.1.0 release.The goal is to prove the async client architecture end to end without changing how the existing synchronous
Mlbclient works.This is intentionally a smaller first pass so we can make sure lifecycle, transport ownership, parsing, and sync compatibility are right before expanding async endpoint support.
Closes #303
What
Added
AsyncMlbas a public package import with async context manager support and explicitaclose()cleanup.Added the initial async endpoint support for:
get_teamget_personget_scheduleI also kept
get_teamsandget_peoplesince they already fit the same shared parsing path and match the existing sync request shape.The async methods reuse the existing models and shared parsing instead of maintaining separate async parsing logic.
Schedule request parameter construction was also moved out of the parser layer into a private helper so the parser remains focused on response data to model conversion.
Caller supplied async clients remain caller owned, while library created clients are cleaned up by
AsyncMlb.Existing synchronous behavior is unchanged.
Tests
Added focused deterministic coverage for the public
AsyncMlbbehavior, including:Package root import
Async context manager lifecycle and cleanup
Caller owned client behavior
Idempotent
aclose()get_team,get_person, andget_scheduleEmpty and 404 behavior
Sync compatible method signatures
Same client concurrent requests
The intent here is to test the
AsyncMlblayer without duplicating the larger transport contract already covered by #302.Validation:
poetry run pytest tests/test_async_mlb.py poetry run pytest tests/ --ignore=tests/external_tests git diff --check Risk and impact Normal This adds a new public async client, so there is some risk around lifecycle behavior, ownership, and keeping the async API aligned with the existing sync API. The change is additive though. Existing Mlb users should not need to change anything when moving from 1.0.x to 1.1.0. If something does go wrong, the main impact should be limited to users of the new AsyncMlb client. Existing synchronous usage should continue to behave the same.