feat: add limit/pagination to mlbplayers endpoint (#141) #162

Claude · 2026-03-24T09:32:26Z

Claude commented

2026-03-24 09:32:26 +00:00

Closes #141

Adds limit: int = 100 query parameter to GET /api/v2/mlbplayers. The limit is capped via max(0, min(limit, 500)) to guard against negative values and enforce the 500-row upper bound. Applied after all filters and before the if csv: branch so both CSV and JSON paths honour the limit.

Files changed:

app/routers_v2/mlbplayers.py

Other observations:

Pre-existing ruff F541 on line 228 (f"POST /mlbplayers/one - dupes found:" — f-string without placeholder) was blocking the pre-commit hook; fixed as a required side-effect of committing.

Closes #141 Adds `limit: int = 100` query parameter to `GET /api/v2/mlbplayers`. The limit is capped via `max(0, min(limit, 500))` to guard against negative values and enforce the 500-row upper bound. Applied after all filters and before the `if csv:` branch so both CSV and JSON paths honour the limit. **Files changed:** - `app/routers_v2/mlbplayers.py` **Other observations:** - Pre-existing ruff F541 on line 228 (`f"POST /mlbplayers/one - dupes found:"` — f-string without placeholder) was blocking the pre-commit hook; fixed as a required side-effect of committing.

Claude added 1 commit 2026-03-24 09:32:28 +00:00

feat: add limit/pagination to mlbplayers endpoint (#141 ) 9391591263

Closes #141

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

Claude referenced this pull request

2026-03-24 09:32:33 +00:00

feat: add limit/pagination to mlbplayers endpoint #141

Claude added the

ai-reviewing

label 2026-03-24 09:45:46 +00:00

Claude reviewed 2026-03-24 09:46:35 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/routers_v2/mlbplayers.py (modified)

Findings

Correctness

limit: int = 100 added to GET /api/v2/mlbplayers — correct form, matching the pattern established in PRs #157, #159, #161.
all_players = all_players.limit(max(0, min(limit, 500))) placed after all eight filter branches (full_name, first_name, last_name, key_fangraphs, key_bbref, key_retro, key_mlbam, offense_col) and before the if csv: branch — both CSV and JSON paths honour the limit. ✅
Negative-value guard (max(0, ...)) and upper-bound cap (min(..., 500)) are correct.
ruff F541 fix at line 228: f"POST /mlbplayers/one - dupes found:" → plain string — correct, no placeholder was present.

Security

No issues found. Limit is an integer capped server-side; no injection surface.

Style & Conventions

MINOR: limit: int (not Optional[int]) — cosmetic inconsistency with some earlier paginated endpoints (e.g., #154, #155, #156, #158), but consistent with the three most recent ones (#157, #159, #161). Both are functionally identical in FastAPI; non-blocking.
MINOR: count in the JSON response reflects the limited count, not the total matching rows. This is the pre-existing pattern across 11 of 12 paginated endpoints (only gauntletruns in PR #160 diverges). Non-blocking, but a follow-up issue to add a total field project-wide would benefit pagination callers.
MINOR: limit=0 returns an empty response even when records exist (max(0, 0) = 0). Same pedantic edge case present in all prior pagination PRs; non-blocking.

Suggestions

This endpoint has several key-lookup filters (key_mlbam, key_bbref, etc.) that callers sometimes use to retrieve a known set of players. A caller fetching 150 specific players by key_mlbam will silently receive only 100 with the default limit. Worth documenting in API docs or raising limit to 500 default for this endpoint specifically — but this is a design preference, not a correctness issue.

Verdict: COMMENT

Implementation is correct and consistent with the established pagination pattern across the codebase. Posting as COMMENT rather than APPROVED due to Gitea's self-review restriction.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/mlbplayers.py` (modified) ### Findings #### Correctness - `limit: int = 100` added to `GET /api/v2/mlbplayers` — correct form, matching the pattern established in PRs #157, #159, #161. - `all_players = all_players.limit(max(0, min(limit, 500)))` placed after all eight filter branches (full_name, first_name, last_name, key_fangraphs, key_bbref, key_retro, key_mlbam, offense_col) and before the `if csv:` branch — both CSV and JSON paths honour the limit. ✅ - Negative-value guard (`max(0, ...)`) and upper-bound cap (`min(..., 500)`) are correct. - ruff F541 fix at line 228: `f"POST /mlbplayers/one - dupes found:"` → plain string — correct, no placeholder was present. #### Security - No issues found. Limit is an integer capped server-side; no injection surface. #### Style & Conventions - **MINOR**: `limit: int` (not `Optional[int]`) — cosmetic inconsistency with some earlier paginated endpoints (e.g., #154, #155, #156, #158), but consistent with the three most recent ones (#157, #159, #161). Both are functionally identical in FastAPI; non-blocking. - **MINOR**: `count` in the JSON response reflects the limited count, not the total matching rows. This is the pre-existing pattern across 11 of 12 paginated endpoints (only gauntletruns in PR #160 diverges). Non-blocking, but a follow-up issue to add a `total` field project-wide would benefit pagination callers. - **MINOR**: `limit=0` returns an empty response even when records exist (`max(0, 0) = 0`). Same pedantic edge case present in all prior pagination PRs; non-blocking. #### Suggestions - This endpoint has several key-lookup filters (`key_mlbam`, `key_bbref`, etc.) that callers sometimes use to retrieve a known set of players. A caller fetching 150 specific players by `key_mlbam` will silently receive only 100 with the default limit. Worth documenting in API docs or raising `limit` to 500 default for this endpoint specifically — but this is a design preference, not a correctness issue. ### Verdict: COMMENT Implementation is correct and consistent with the established pagination pattern across the codebase. Posting as COMMENT rather than APPROVED due to Gitea's self-review restriction. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-24 09:46:55 +00:00

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

cal left a comment

Implementation is correct and consistent with the pagination pattern established across the codebase. The limit guard (max(0, min(limit, 500))) is properly placed after all filters and before the csv/json branch split. The ruff F541 fix is a clean side-effect. Approving.

Implementation is correct and consistent with the pagination pattern established across the codebase. The limit guard (`max(0, min(limit, 500))`) is properly placed after all filters and before the csv/json branch split. The ruff F541 fix is a clean side-effect. Approving.

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

cal left a comment

Implementation is correct and consistent with the pagination pattern established across the codebase. Approving.

cal added 1 commit 2026-03-24 12:05:15 +00:00

Merge branch 'main' into issue/141-feat-add-limit-pagination-to-mlbplayers-endpoint 7c69a56b30

cal added 1 commit 2026-03-24 12:06:33 +00:00

Merge branch 'main' into issue/141-feat-add-limit-pagination-to-mlbplayers-endpoint 3a65d84682

cal merged commit 9a320a0e4e into main

2026-03-24 12:06:50 +00:00

cal deleted branch issue/141-feat-add-limit-pagination-to-mlbplayers-endpoint

2026-03-24 12:06:50 +00:00

cal referenced this issue from a commit

2026-03-24 12:06:51 +00:00

Merge pull request 'feat: add limit/pagination to mlbplayers endpoint (#141)' (#162) from issue/141-feat-add-limit-pagination-to-mlbplayers-endpoint into main

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#162