Load functions #130
Load functions #130
Conversation
Codecov Report
@@ Coverage Diff @@
## main #130 +/- ##
==========================================
+ Coverage 87.93% 88.02% +0.09%
==========================================
Files 39 41 +2
Lines 2784 2806 +22
==========================================
+ Hits 2448 2470 +22
Misses 336 336
Continue to review full report at Codecov.
|
|
Since when did checks fail for warnings?! Tried fixing this issue with |
| test_df <- load_match_results(country = "ENG", gender = "M", season_end_year = 2022, tier="1st") | ||
| # test the functions returns the data | ||
| expect_type(test_df, "list") | ||
|
|
There was a problem hiding this comment.
Maybe add expect_false(nrow(test_df) == 0)?
There was a problem hiding this comment.
Yeah I like it. I'll make the changes
|
|
||
| This is the load function equivalent of `get_match_results()`. | ||
|
|
||
| ```{r load_match_results, eval=FALSE} |
There was a problem hiding this comment.
Because the vignette loads the library at the start, then fails to recognise these new functions as they're not yet deployed to the library. So once the version gets updated, then I go in and rebuild that one article and set eval=TRUE...
Unless I've got that completely wrong?
There was a problem hiding this comment.
hmm i think you can run devtools::install() prior to running pkgdown::build_site() to ensure that the new functions are available
There was a problem hiding this comment.
if we weren't ignoring vignettes with .Rbuildignore, I believe the functions would be available to the vignettes anyways. i think the package gets built before the vignettes
R/load_fb.R
Outdated
| load_match_results <- function(country, gender, season_end_year, tier) { | ||
| dat_urls <- paste0("https://github.com/JaseZiv/worldfootballR_data/blob/master/data/match_results/", country, "_match_results.rds?raw=true") | ||
|
|
||
| collect_date <- .file_reader("https://github.com/JaseZiv/worldfootballR_data/blob/master/data/match_results/scrape_time_match_results.rds?raw=true") |
There was a problem hiding this comment.
i'm not sure i like this paradigm (having a separate data set for scrape times). what if we tried saving the timestamp as an attribute with the data set? e.g. https://github.com/nflverse/nflfastR-data/blob/91bb21f44596b474f2c4390e348c8ac44c24879c/R/save_stats.R#L13 I'm not totally sure if this is possible with RDS (i think it probably is)
There was a problem hiding this comment.
Have addressed this in 6fe808c
Fix typos in vignettes Co-authored-by: Tony ElHabr <[email protected]>
| dat_df <- dat_df %>% | ||
| dplyr::filter(.data$Season_End_Year %in% season_end_year) | ||
|
|
||
| cli::cli_alert("Data last updated {attr(dat_df, 'scrape_timestamp')} UTC") |
No description provided.