-
Notifications
You must be signed in to change notification settings - Fork 51
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
Use default play to predefine column types in update_db()
#325
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you prefix this with any::tidyverse? https://github.com/nflverse/nflfastR/blob/master/.github/workflows/pkgdown.yaml#L34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also just needs to hit redocument to bring in the redact_path arg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, maybe josh would be willing to test new methodology
nflreadr sitrep is redocumented. I had to update nflreadr first smh. |
|
Testing now! |
That's actually wrong. The first season written to the table is combined with the default play. So we have a complete season for the datatype method to set the types. |
Ok, it's definitely broken because I did something stupid, hold on |
from 4.3.0.9011
|
That's interesting. So nflfastR calls |
OK, I tried a fresh update to a new table and got the same error.
However, |
This resolves #324
Added a new object to sysdata which is a single tibble row that uses random strings for character types with maximum length across the complete dataset (as of 2021).
This should make db drivers to correctly set datatypes in
update_db()
esp. for long string columns.It is definitely possible that this needs an update at some point, if one or more of the defined string columns isn't big enough.
Thanks to @friscojosh for the idea. I hope this works.