-
Notifications
You must be signed in to change notification settings - Fork 190
Add control over digits and jsonlite options in uploading data #664
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
base: main
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| fields <- lapply(x, format) | ||
| dig <- getOption("bigrquery.digits") |
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.
Again, does this need to be a option? Could we just increase the default digits?
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.
Same as #665 (comment), predominantly "backward compatibility". If we discard that, I still think being able to control the number of digits is both idiomatic for R and useful.
For example, in some of my use-cases (using jsonified objects), I need to reduce string-length as much as possible and care only about integers or maybe 1-2 digits, in which case toJSON(digits=NA) is undesired. In others, I really need at least 6 digits. I don't think there's a single number that will be awesome for everybody.
I think a counter to your question of "does this need to be an option" is to change the default value so that most people will not need to set it. This will change the package behavior a little, I'm good with that if you are.
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.
btw, per your other comment #665 (comment), I suggest that adding "max digits" to a POSIXt timestamp has less impact to setting either digits=NA (max) or some less-max hard-coded option. I suggest a default of 7 is better than 4, but I do think an option here is more important than with bigrquery.digits.secs.
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.
Would you prefer digits=NA always or keeping getOption("bigrquery.digits")?
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.
Hmmmm, in that case I think I'd prefer making it an option that's bound to the connection. In general, I don't think it's good practice to have options that affect the results of a computation (as opposed to its display).
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.
A simpler path could be to rename the parameters= args to the bq_* functions to params=, so we wouldn't have the potential for this conflict. What do you think about that?
(wrong discussion)
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.
To be a bit more explicit, you'd need to add a new field to
BigQueryConnectionand then make sure that gets passed around in all the right places.
Are you good with mimicking quiet=getOption("bigrquery.quiet"), an arg with an option default value?
(Sry about the delay, I mixed comment-chains.)
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.
... and is your "arg" point aimed at just bigrquery.digits.secs, or do you want bigrquery.jsonlite.toJSON (or whatever its name should be) to also be an 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.
I'd rather stay focussed on the digits for now, and then tackle the other arbitrary options in a different PR if you think they are really important (which I'm sceptical of).
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.
I often do not like jsonlite's default args; while 99% of the time it's digits=4 that I don't like, I do feel that being able to influence them is important enough. I can break it into a different PR and we'll focus on digits= here. Do you want the arg name to be bq_*(digits=) universally?
Two methods to control digits in uploaded data:
options(bigrquery.jsonlite.toJSON=list(digits=NA))passes that arg (and any others in the named list) tojsonlite::stream_outand::toJSON; andoptions(bigrquery.digits=22), a simpler way to pass only digits to the same functions.Includes
NEWS, code, and unit-tests.