Understat match stats and players #386
Conversation
Stats table function
Players data for each match
match_stats and match_players
Understat match stats and player
There was a problem hiding this comment.
This looks ok to me, and I'll be happy to approve when my feedback is addressed. Oh, and one other thing not mentioned in in-line comments--please add tests for each function in tests/testthat/test-understat.R.
This is a great first pass! I'm really happy to see your contribution.
Understat match review
match_stats details section
|
Great! I have addressed all the comments in the review (namespaces, indentation, and variable coercion). Added the tests for both functions and updated some of the documentation. For The 90% does return correctly (and it is being processed into a
|
thanks for the clarification. that seems fine to me 👍 |
|
i just pushed some final touch-ups. testing in Github Actions now. some transfermarket tests may fail, but we should see the understat tests pass |
| #' | ||
| #' @export | ||
|
|
||
| understat_match_players <- function(match_url) { |
There was a problem hiding this comment.
my only new feedback on this is that there might have been a cleaner way of doing all of this. i noticed that {understatr} seems to have a more straightforward approach, although i haven't thoroughly checked the details. it could be the same exact implementation.
There was a problem hiding this comment.
but i don't think this function should be changed at this point
|
Thanks for the contribution @jruizcabrejos! Thanks also for the work reviewing @tonyelhabr! |
Currently, understat_match data only retrieves shots data.
This 2 functions should retrieve both the Stats table and the player data for each match.