Skip to content

Conversation

tanho63
Copy link
Member

@tanho63 tanho63 commented Jan 20, 2025

No description provided.

@tanho63 tanho63 changed the title clean up internal namespace and directly re-export load_pbp, load_player_stats clean up internal namespace and directly re-export nflreadr functions Jan 20, 2025
Copy link

github-actions bot commented Jan 20, 2025

glue,
janitor,
lifecycle,
magrittr,
Copy link
Member

Choose a reason for hiding this comment

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

I would like to get rid of the magrittr pipe and never imported it because the pipe is reexported in dplyr and we need to watch the number of dependencies

#' @importFrom cli rule
#' @importFrom curl curl_fetch_memory
#' @importFrom nflreadr load_pbp load_player_stats load_schedules load_rosters nflverse_sitrep
#' @importFrom magrittr %>%
Copy link
Member

Choose a reason for hiding this comment

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

import from dplyr

#' @importFrom tidyr unnest_wider unnest
#' @importFrom tidyselect any_of ends_with
#' @importFrom utils packageVersion
#' @importFrom xgboost getinfo
Copy link
Member

Choose a reason for hiding this comment

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

nflfastR needs xgboost to apply the models but there is no actual function call to xgboost which results in a R CMD Check note. That's why I import a dummy.

#' }
#' }
#' @export
report <- function(...) nflreadr::nflverse_sitrep(...)
Copy link
Member

Choose a reason for hiding this comment

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

Not happy with silently dropping the function report completely


#' Load Team Rosters for Multiple Seasons
#' @details See [`nflreadr::load_rosters`] for details.
#' @inheritDotParams nflreadr::load_rosters
Copy link
Member

Choose a reason for hiding this comment

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

This produces in a check warning

@mrcaseb
Copy link
Member

mrcaseb commented Jan 25, 2025

General comment on this is that there are some situations where we didn't namespace the function call. In sum_playstats it's for performance reasons. In other cases probably no reason.

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.

2 participants