-
Notifications
You must be signed in to change notification settings - Fork 340
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
Added docstrings to any undocumented public-facing functions #340
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this is really nice! Thanks for the contribution. I'll give some time for others to comment, since this has been a point of discussion for quite some time now.
pybaseball/amateur_draft_by_team.py
Outdated
Get amateur draft results by team and year. | ||
|
||
ARGUMENTS | ||
team: Team code which you want to check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say "see docs for list of team codes" and even possibly link if not too ugly?
https://github.com/jldbc/pybaseball/blob/master/docs/amateur_draft_by_team.md
pybaseball/statcast_pitcher_spin.py
Outdated
RETURNS | ||
A dataframe containing pitch-level statcast data for the given dates, along with these colummns added: | ||
|
||
Mx: The amount of movement in the x-direction due to the Magnus effect alone. (Positive is towards first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is a lot to have in the code? I could see either way, what do you think? This is another place we could say "see the docs for the added columns."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it is a lot-- I just changed it to a bit of an in between, where I indicate that the other columns exist but leave the descriptions of them to the docs.
Thank you for the quick feedback @tjburch !! I just responded to the initial feedback, and will respond to more as it comes up! |
cool, this lgtm. Appreciate it. I'll leave it to @schorrm to merge, so he at least gets eyes on it since it's been an ongoing discussion. |
LGTM! |
This CI business is getting to be a nightmare. |
Ok, I resolved the CI stuff in another PR. I'll just go ahead and merge this. Since it's only docstrings, I'm not too worried about it. Thanks for the contribution @erin2722! |
Based on the discussion in #298, I combed through the codebase and added docstrings to any public-facing functions that did not already have them.
Let me know if this was overkill, and I can go back through and remove some docstrings, or change the formatting!