Skip to content

convert floating point stats from string - #241

Open
louisadamian wants to merge 1 commit into
zero-sum-seattle:mainfrom
louisadamian:convert_float_stats
Open

convert floating point stats from string#241
louisadamian wants to merge 1 commit into
zero-sum-seattle:mainfrom
louisadamian:convert_float_stats

Conversation

@louisadamian

Copy link
Copy Markdown
Contributor

Convert floating stats with decimal representation from str to float with pydantic

adds a BeforeValidator to base.py that converts types to float or returns none and an OptionalFloat type that used the validator and changed all the relevant types in stat objects.

Tests

Passes all unit tests.

@Mattsface

Copy link
Copy Markdown
Member

I've gone ahead and removed the broken workflows and scripts that was causing the CI failure.

Can you pull them down for the PR and run it?

Also thank you for the PR :)

@louisadamian

Copy link
Copy Markdown
Contributor Author

Thanks, I merged in your upstream changes, but I can't trigger any workflows.

@louisadamian

Copy link
Copy Markdown
Contributor Author

@Mattsface I've rebased on top of 0.8.0 all tests are passing

@Mattsface

Copy link
Copy Markdown
Member

@claude review please

@claude

claude Bot commented Aug 16, 2026

Copy link
Copy Markdown

Claude finished @Mattsface's task in 2m 20s —— View job


Code Review

  • Gather context (PR diff, files changed)
  • Review mlbstatsapi/models/base.py validator/type changes
  • Review stat model changes (catching, fielding, hitting, pitching)
  • Review test changes
  • Check for downstream code that assumes these fields are strings
  • Post review feedback

CI (Offline CI) is passing on this PR. Note: I wasn't able to execute the local test suite myself in this sandbox (bash execution requiring approval was denied), so this review is based on static analysis.

Summary: Solid, low-risk change. The OptionalFloat type (Annotated[float | None, BeforeValidator(float_or_none)] in mlbstatsapi/models/base.py:10-17) is a clean, reusable way to coerce string decimal stats to float, silently falling back to None on unparseable values (empty strings, None, MLB's .---/-.-- placeholders, etc.). Applied consistently across catching.py, fielding.py, hitting.py, and pitching.py. I searched the rest of mlbstatsapi/ for code that treats these fields as strings (formatting, slicing, serialization) and found none — the change is contained to the model layer.

A few things worth addressing:

  1. ExpectedStatistics (mlbstatsapi/models/stats/stats.py:35-53) was missed. It has the same "formatted decimal string" semantics (avg, slg, woba, wobacon) as the fields converted elsewhere in this PR, but is still declared as required str. This is inconsistent with the PR's goal, and since the fields are required (not Optional), a missing value in the API response would raise a validation error rather than degrade gracefully like the newly-converted fields do.
    Fix this →

  2. README.md example output is now stale. README.md:437-440 shows print(f"Batting Avg: {split.stat.avg}")Batting Avg: .274. Since SimpleHittingSplit.avg (mlbstatsapi/models/stats/hitting.py:206) is now a float, the real output is Batting Avg: 0.274, not .274. (The other doc example at README.md:487-490 using ExpectedStatistics is unaffected since that class wasn't converted — see point 1.)
    Fix this →

  3. Stale docstrings in hitting.py still describe the converted fields as str (e.g. babip : str at line 29, avg : str at line 147, obp/slg/ops etc.), while the equivalent docstrings in pitching.py, catching.py, and fielding.py were already updated to say float. Cosmetic, but worth a pass for consistency.

  4. Semantic note (not a blocker): MLB's innings fields (innings_pitched in pitching.py:175, innings in fielding.py:80) use a notation where the digit after the decimal represents thirds of an inning (e.g. "6.2" means 6⅔ innings, not 6.2). This ambiguity pre-dates the PR (it was already a string), but converting to a real float may more strongly invite downstream arithmetic that assumes true decimal semantics. Worth a docstring callout if not already documented elsewhere.

None of these block the PR — 1 and 2 are the most worth fixing before merge since they're small, concrete gaps; 3 and 4 are minor polish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants