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 kicking stats to ncaaf players #381

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

AidanGlickman
Copy link
Contributor

Fixes #373

Adds kicking stats (attempts and percentage) to NCAAF players.

Not sure how the previous PR #374 broke but here it is again?

@roclark roclark self-requested a review April 5, 2020 20:43
@roclark roclark added the enhancement New feature or request label Apr 5, 2020
@roclark roclark added this to the Release 0.5.1 milestone Apr 5, 2020
Copy link
Owner

@roclark roclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AidanGlickman for putting this together! This looks good! If you add the note about the percentage range for the one attribute, I can get this merged. Also, per your comments on #374 on testing, perhaps we could get this in a good position where it's ready to merge, and I can create another PR with an update to the testing suite, then merge them sequentially. How does that sound? Or would it be easiest if I create a branch upstream, and you base this PR off that branch to include the changes into a single commit? That might look more elegant in the end, but is a more involved process.

Either way, I can work on the testing changes on my end to get these changes included with the testing suite moving forward.

Thanks again for this PR! This is wonderful!

sportsreference/ncaaf/player.py Outdated Show resolved Hide resolved
@AidanGlickman AidanGlickman force-pushed the add-ncaaf-kicking branch 3 times, most recently from 8c3d4bb to ef08278 Compare April 5, 2020 22:11
@roclark
Copy link
Owner

roclark commented Apr 6, 2020

Hey @AidanGlickman, I added a branch which includes updates to the test files to properly include this with the testing suite. If you wouldn't mind, it would be great if you could copy the test file and updates I created and include them with this commit. It looks like everything passes the tests on my end, but I want to make sure you have the proper credit for this, so feel free to copy everything as-is.

Let me know if you have any questions or concerns. Otherwise, once the tests are included, I will merge this and include it with my upcoming release!

@AidanGlickman
Copy link
Contributor Author

Sounds good! I went ahead and added them to the commit. I look forward to contributing more where I can!

@roclark
Copy link
Owner

roclark commented Apr 6, 2020

Thanks again @AidanGlickman! This looks great and I will merge it! Thank you very much for the addition and for the changes! I will work on cutting a new release now so this can be included with all new binary pulls.

@roclark roclark merged commit ee08093 into roclark:master Apr 6, 2020
@AidanGlickman AidanGlickman deleted the add-ncaaf-kicking branch April 8, 2020 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Kicking table stats for NCAAF players
2 participants