Skip to content

Conversation

@mrcaseb
Copy link
Member

@mrcaseb mrcaseb commented Oct 20, 2025

As discussed in #543

This adds the new function update_pbp_db() with a more intuitive and consistent api.

A few notes:

  • The function name is a placeholder atm.
  • The new function api requires a database connection and a table name, as in almost all DBI functions.
  • No more default DB driver
  • No more path handling
  • The seasons argument works similar as in all nflreadr functions
  • It is now possible to build the DB with selected seasons (that's especially useful for the vignette where we currently build the complete database with every run)
  • Seasons can be written in chunks, which is more efficient than the previous solution

@github-actions
Copy link

github-actions bot commented Oct 20, 2025

as it creates a R CMD Check warning because of package links
@mrcaseb
Copy link
Member Author

mrcaseb commented Oct 20, 2025

@guga31bb and @tanho63 this is what I had in mind as fresh approach for the db builder. More consistent and intuitive api imo.

Please have a look

@mrcaseb
Copy link
Member Author

mrcaseb commented Oct 20, 2025

To-do:

  • Fix typos
  • Add test for structure of default_play

@mrcaseb mrcaseb requested review from guga31bb and tanho63 October 23, 2025 12:38
@mrcaseb
Copy link
Member Author

mrcaseb commented Oct 23, 2025

I am definitely not all in on the new function name and open to suggestions. I think we should keep update_db as is and start a deprecate process in the offseason.

@tanho63
Copy link
Member

tanho63 commented Oct 23, 2025

I am definitely not all in on the new function name and open to suggestions. I think we should keep update_db as is and start a deprecate process in the offseason.

Strategywise it might make sense to make it a dot-prefixed unexported function and then swap them in the offseason?

@mrcaseb
Copy link
Member Author

mrcaseb commented Oct 23, 2025

I am definitely not all in on the new function name and open to suggestions. I think we should keep update_db as is and start a deprecate process in the offseason.

Strategywise it might make sense to make it a dot-prefixed unexported function and then swap them in the offseason?

you mean the new one should take the old one's name? The api changed so this would break code but I guess we could do that during the offseason

Copy link
Member

@tanho63 tanho63 left a comment

Choose a reason for hiding this comment

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

so what's the difference between the two in terms of practical user experience, can they drop in a table / connection to their existing db?

@mrcaseb
Copy link
Member Author

mrcaseb commented Oct 23, 2025

so what's the difference between the two in terms of practical user experience, can they drop in a table / connection to their existing db?

A db connection is mandatory now and I have renamed the default table to "nflverse_pbp" instead of the old "nflfastR_pbp". Users can overwrite this with the name arg.
It is possible to connect an existing db (and table) and let the new function work with it.

mrcaseb and others added 2 commits October 23, 2025 16:07
Co-authored-by: Tan Ho <[email protected]>
to make messaging work if `name` is a call to `DBI::SQL()` or `DBI::Id()`
@mrcaseb mrcaseb requested a review from tanho63 October 23, 2025 15:41
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