feat: add limit/pagination to battingcardratings endpoint (#135) #159

Claude · 2026-03-24T08:03:24Z

Claude commented

2026-03-24 08:03:24 +00:00

Closes #135

Adds optional limit query parameter (default 100, max 500) to GET /api/v2/battingcardratings.

Changes

Added limit: int = 100 parameter to get_card_ratings()
Applied all_ratings.limit(max(0, min(limit, 500))) after all filters, before the if csv: branch — both CSV and JSON paths honour the limit
Ruff auto-fixed 10 pre-existing F541 (f-string without placeholders) violations required by the pre-commit hook

Files changed

app/routers_v2/battingcardratings.py

Closes #135 Adds optional `limit` query parameter (default 100, max 500) to `GET /api/v2/battingcardratings`. ## Changes - Added `limit: int = 100` parameter to `get_card_ratings()` - Applied `all_ratings.limit(max(0, min(limit, 500)))` after all filters, before the `if csv:` branch — both CSV and JSON paths honour the limit - Ruff auto-fixed 10 pre-existing F541 (f-string without placeholders) violations required by the pre-commit hook ## Files changed - `app/routers_v2/battingcardratings.py`

Claude added 1 commit 2026-03-24 08:03:25 +00:00

feat: add limit/pagination to battingcardratings endpoint (#135 ) dc88b1539c

Closes #135

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Claude referenced this pull request

2026-03-24 08:03:30 +00:00

feat: add limit/pagination to battingcardratings endpoint #135

Claude added the

ai-reviewing

label 2026-03-24 08:15:47 +00:00

Claude reviewed 2026-03-24 08:16:45 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/routers_v2/battingcardratings.py (modified)

Findings

Correctness

limit: int = 100 added to get_card_ratings() — correct default and type.
all_ratings = all_ratings.limit(max(0, min(limit, 500))) applied after all three filter branches (battingcard_id, vs_hand, cardset_id) and before the if csv: branch — both CSV and JSON response paths honour the limit correctly. ✅
Ruff F541 fixes (10 f-strings without placeholders → plain strings) are correct and don't affect behaviour.
Query reformatting in get_player_ratings() (lines 670–673) is a style-only change; logic is unchanged.

Security

No issues. Auth (team_id + ts hash check) is enforced at the top of get_card_ratings() before the limit is applied. The limit parameter is fully bounds-checked before reaching the ORM.

Style & Conventions

MINOR (non-blocking): limit: int = 100 is used here, while most other paginated endpoints in this series use limit: Optional[int] = 100. PR #157 (awards) also used plain int. Both behave identically in FastAPI (the parameter is always present with a default), so this is a cosmetic inconsistency only.

Suggestions

MINOR: count in the JSON response reflects the limited row count, not the total matching rows. This is the pre-existing pattern across all 9 paginated endpoints — non-blocking, but a follow-up issue to add a total field (or rename count) project-wide would improve pagination caller UX.
MINOR: limit=0 returns an empty response even when records exist (max(0, min(0, 500)) = 0 → LIMIT 0). Same pedantic edge case present in all prior pagination PRs; a lower bound of 1 would prevent it but is not required.

Verdict: COMMENT

Implementation is correct. Limit is applied in the right place, bounds-checked identically to the other 8 paginated endpoints, and both CSV and JSON paths honour it. Minor observations (int vs Optional[int] type, count semantics, limit=0 edge case) are pre-existing patterns across the series — none block merging. Posting as COMMENT rather than APPROVED due to Gitea self-review restriction.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/battingcardratings.py` (modified) ### Findings #### Correctness - `limit: int = 100` added to `get_card_ratings()` — correct default and type. - `all_ratings = all_ratings.limit(max(0, min(limit, 500)))` applied after all three filter branches (`battingcard_id`, `vs_hand`, `cardset_id`) and before the `if csv:` branch — both CSV and JSON response paths honour the limit correctly. ✅ - Ruff F541 fixes (10 f-strings without placeholders → plain strings) are correct and don't affect behaviour. - Query reformatting in `get_player_ratings()` (lines 670–673) is a style-only change; logic is unchanged. #### Security - No issues. Auth (`team_id` + `ts` hash check) is enforced at the top of `get_card_ratings()` before the limit is applied. The `limit` parameter is fully bounds-checked before reaching the ORM. #### Style & Conventions - **MINOR (non-blocking):** `limit: int = 100` is used here, while most other paginated endpoints in this series use `limit: Optional[int] = 100`. PR #157 (awards) also used plain `int`. Both behave identically in FastAPI (the parameter is always present with a default), so this is a cosmetic inconsistency only. #### Suggestions - **MINOR:** `count` in the JSON response reflects the limited row count, not the total matching rows. This is the pre-existing pattern across all 9 paginated endpoints — non-blocking, but a follow-up issue to add a `total` field (or rename `count`) project-wide would improve pagination caller UX. - **MINOR:** `limit=0` returns an empty response even when records exist (`max(0, min(0, 500))` = 0 → `LIMIT 0`). Same pedantic edge case present in all prior pagination PRs; a lower bound of 1 would prevent it but is not required. ### Verdict: COMMENT Implementation is correct. Limit is applied in the right place, bounds-checked identically to the other 8 paginated endpoints, and both CSV and JSON paths honour it. Minor observations (int vs Optional[int] type, count semantics, limit=0 edge case) are pre-existing patterns across the series — none block merging. Posting as COMMENT rather than APPROVED due to Gitea self-review restriction. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-24 08:17:01 +00:00

cal approved these changes 2026-03-24 12:04:39 +00:00

cal left a comment

Implementation is correct. Limit is properly bounds-checked, applied before both CSV and JSON response paths, and follows the established pattern across all paginated endpoints. Minor observations from the automated review are pre-existing patterns — nothing blocks merging. Approving as cal.

cal added 1 commit 2026-03-24 12:04:58 +00:00

Merge branch 'main' into issue/135-feat-add-limit-pagination-to-battingcardratings-en cf668703ed

cal merged commit 5fba31a325 into main

2026-03-24 12:05:38 +00:00

cal referenced this issue from a commit

2026-03-24 12:05:39 +00:00

Merge pull request 'feat: add limit/pagination to battingcardratings endpoint (#135)' (#159) from issue/135-feat-add-limit-pagination-to-battingcardratings-en into main

cal deleted branch issue/135-feat-add-limit-pagination-to-battingcardratings-en

2026-03-24 12:05:39 +00:00

Sign in to join this conversation.

No reviewers

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/paper-dynasty-database#159