jldbc / pybaseball

Pull current and historical baseball statistics using Python (Statcast, Baseball Reference, FanGraphs)
MIT License
1.18k stars 323 forks source link

Feat: Update "OOA Behind Pitcher" function #299

Closed zach-hopkins closed 1 year ago

zach-hopkins commented 1 year ago

Proposed feature which simply adds an argument to the "pitcher_fielding" statcast pull in order to allow for "view" changes (options are: Fielder, Fielding Team, Batting, Batting Team, Pitcher). Helpful for pulling OOA and RAA for defense behind a particular pitcher and retrieving team fielding information. Set default to previous hardcode "Fielder"

schorrm commented 1 year ago

Looks good.

  1. Move the new argument to be last for now? @tjburch do we care that much about breaking compat here
  2. Add to tests?
  3. Is there anything that needs updates for the docs?
tjburch commented 1 year ago

Hey, thanks Zach, this is a clever addition to make this more useful.

Agree with @schorrm on moving the argument to the end. I added a review about a docstring so we know what all can be put there.

For the test, make sure this one still runs with the default value, then add a second just like it for a different option of view.

For the docs, just add a note in this file with the alternate options for views.

Thanks again!

zach-hopkins commented 1 year ago

Hey, thanks Zach, this is a clever addition to make this more useful.

Agree with @schorrm on moving the argument to the end. I added a review about a docstring so we know what all can be put there.

For the test, make sure this one still runs with the default value, then add a second just like it for a different option of view.

For the docs, just add a note in this file with the alternate options for views.

Thanks again!

No problem! Added the changes, let me know if all looks right. Works well on my end. 6d36b9c

schorrm commented 1 year ago

LGTM for me

tjburch commented 1 year ago

And also me. Seems pointless to run tests right now since they're all broke anyway so smashing that merge button.

Thanks a ton @zach-hopkins!