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 2 new FBref load functions #330

Merged
merged 2 commits into from
Sep 21, 2023
Merged

Add 2 new FBref load functions #330

merged 2 commits into from
Sep 21, 2023

Conversation

tonyelhabr
Copy link
Collaborator

Adds load_fb_match_summary() and load_fb_advanced_match_stats().

Appendix

  • Unfortunately, I initiated the data scraped prior to Add column for player ID to 3 FBref functions #328, so I'll need to backfill at some point. Nonetheless, there are many seasons and leagues worth of data already scraped and published in the worldfootballR_data repo.
  • Long term, we may consider consolidating the logic for the load_ functions, since most follow the same pattern.

@codecov-commenter
Copy link

Codecov Report

Merging #330 (f1bb257) into main (e508a1e) will increase coverage by 0.34%.
Report is 4 commits behind head on main.
The diff coverage is 98.27%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #330      +/-   ##
==========================================
+ Coverage   62.83%   63.18%   +0.34%     
==========================================
  Files          44       44              
  Lines        3552     3583      +31     
==========================================
+ Hits         2232     2264      +32     
+ Misses       1320     1319       -1     
Files Changed Coverage Δ
R/fb_player_match_logs.R 96.66% <96.42%> (+0.05%) ⬆️
R/load_fb.R 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Owner

@JaseZiv JaseZiv left a comment

Choose a reason for hiding this comment

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

lgtm.

Agree with your point about consolidation at some point.

Have left this unmerged in case you needed to add anything to it. If not, merge it whenever you're ready.

Thanks heaps @tonyelhabr

@tonyelhabr tonyelhabr merged commit 8410368 into main Sep 21, 2023
2 checks passed
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.

3 participants