feat: add per-endpoint rate limiting#156
Merged
NandaScott merged 19 commits intodevelopfrom Apr 7, 2026
Merged
Conversation
Scryfall now enforces tiered rate limits: 2/s for /cards/search, /cards/named, /cards/random, /cards/collection, and 10/s for all other endpoints. The four slow endpoints share a single 2/s budget. - Add SlowRateLimiter subclass with 2.0 calls/second default - Change RateLimiter singleton to per-class registry so each rate tier maintains independent state - Add _rate_limiter_class to ScrythonRequestHandler so endpoint classes declare their rate tier - Assign SlowRateLimiter to Search, Named, Random, Collection - rate_limit_per_second kwarg still overrides per-request - rate_limit=False still disables entirely - No public API changes; existing user code works without modification
This comment was marked as outdated.
This comment was marked as outdated.
The rate_limit_per_second kwarg was creating a new RateLimiter per request, so consecutive calls never throttled against each other. Use a shared registry keyed by (class, rate) so custom rates enforce across calls. Also rename reset_global_limiter to reset_all_limiters to reflect that it clears the entire registry.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Instead of caching override limiters in the global registry (which grows unbounded with unique rate values), store the override limiter on the handler instance. This keeps pagination throttled within a single handler while avoiding an unbounded global cache. Removes get_global_limiter_for_rate() and simplifies the registry type back to dict[type, RateLimiter].
Lower bounds prove correctness (rate limiting works). Upper bounds only assert "not absurdly slow" and should be generous enough to pass on heavily loaded CI runners.
This comment was marked as outdated.
This comment was marked as outdated.
Add __init__ docstring explaining that rate_limit_per_second creates a per-instance limiter scoped to the handler's lifecycle. Also widen test_rate_limit_can_be_disabled upper bound for CI reliability.
This comment was marked as outdated.
This comment was marked as outdated.
…ide throttling - get_global_limiter() now accepts the old calls_per_second param with a DeprecationWarning (previously would TypeError) - Add test proving per-instance override limiter throttles repeated calls within the same handler (e.g., pagination) - Remove stale rate_limit_per_second reference from iter_all docstring
This comment was marked as outdated.
This comment was marked as outdated.
The kwarg is only effective at construction time. Passing it to iter_all() was silently ignored. Now emits a UserWarning and strips the kwarg. Also adds a note to the README that the override is scoped to the handler instance.
This comment was marked as outdated.
This comment was marked as outdated.
Add 2.0.1 changelog entry documenting new rate limiting features, behavioral changes, and deprecations. Update CLAUDE.md to reflect built-in rate limiting, correct Python version, and add rate_limiter and cache modules to the file structure.
…teError on from_dict instances
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
test comment - ignore |
|
Code Review - Part 1 of 5: Overall, this is a well-structured addition. The per-class registry design is clean, the deprecation warnings are properly implemented, and the test coverage is thorough. A few things worth flagging before merging (continued in follow-up comments): |
|
Bug: _override_limiter always creates base RateLimiter - In scrython/base.py, when rate_limit_per_second is provided, the override limiter is always a RateLimiter instance regardless of the endpoint's _rate_limiter_class. This means the override limiter ignores any future behavior added to SlowRateLimiter subclasses. A more consistent approach would be: self._override_limiter = self._rate_limiter_class(rate_limit_per_second). At minimum this should be documented. |
|
deleting test |
|
test - ignore |
1 similar comment
|
test - ignore |
This comment was marked as resolved.
This comment was marked as resolved.
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.
Scryfall API rate limit docs: https://scryfall.com/docs/api/rate-limits
Summary
Test plan