Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Negro Leagues To FangraphsLeague #334

Merged
merged 3 commits into from Mar 10, 2023

Conversation

nwinston
Copy link
Contributor

@nwinston nwinston commented Mar 8, 2023

Now that Fangraphs supports negro leagues, we should be able to pull that data through pybaseball. New values need to be added to the FangraphsLeague enum otherwise an exception is thrown.

@nwinston nwinston closed this Mar 8, 2023
@nwinston nwinston reopened this Mar 8, 2023
@nwinston nwinston marked this pull request as ready for review March 8, 2023 04:49
@BrayanMnz
Copy link
Contributor

Looks good to me. @tjburch

@tjburch
Copy link
Collaborator

tjburch commented Mar 9, 2023

thanks @nwinston, this will be really good to add.

Do you think you could add comments to all the docs where this might be used? E.g. in team_batting.md could change

``league:String. Either "all", "nl", or "al" for determining whether you want data from one league or both. Defaults to "all", for returning data on all teams.

to

``league:String. Either "all" for all data, "nl" for the National League, "al" for the American League, or "mnl" for the Negro Leagues. Defaults to "all", for returning data on all teams.

@nwinston
Copy link
Contributor Author

nwinston commented Mar 9, 2023

Done! @tjburch

@tjburch
Copy link
Collaborator

tjburch commented Mar 10, 2023

Thanks. LGTM! Thanks a ton. Will give a day for anyone else to make comments and merge after. ( @schorrm )

@schorrm
Copy link
Collaborator

schorrm commented Mar 10, 2023

Looks amazing! (thanks for tagging me @tjburch)
One question though: is there a reason why the param can only be 'mnl'?

@nwinston
Copy link
Contributor Author

Updated the docs in fangraphs.md that says to look at FangraphsLeague for the full set. I'll include that note in the rest of them as well.

@schorrm
Copy link
Collaborator

schorrm commented Mar 10, 2023

Anyway, more than LGTM, @tjburch merge whenever you want

@tjburch tjburch merged commit a0e5c4a into jldbc:master Mar 10, 2023
@tjburch
Copy link
Collaborator

tjburch commented Mar 10, 2023

In. Thanks again @nwinston 👍👍

@nwinston
Copy link
Contributor Author

No problem!

nwinston added a commit to nwinston/pybaseball that referenced this pull request Mar 10, 2023
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.

None yet

4 participants