Enforce integer during pagination

[awong234]

See issue 71.

box_ls et cetera return HTTP status 400: Bad Request during box_pagination execution. The issue was found to be that R explicitly uses scientific notation when the offset (or any number) >= 100,000; below this, I suppose it supplies an integer or something recognizable by the Box API as an integer.

The only relevant change here is line 57 in boxr_misc.R, within box_pagination().

-url$query$offset <- (page - 1) * limit
+url$query$offset <- as.integer((page - 1) * limit)

The other stuff is just ad hoc functions I included, but may be valuable for users with large data storage; box_ls_short just queries the "name" field from the folder, speeding up the query by about 35%. Do you have any other suggestions for speeding up box_ls when the directory is big (>1e5 files)?

Sorry if it's a little messy with all the files being changed, I don't have much experience with Roxygen2 to selectively update the stuff, but I imagine you can easily change things here and there on your end.

[ijlyttle]

Hi @awong234

Nice catch!

I'm just waiting for some restarted tests to pass, but I expect I will merge this shortly.

Thanks!

PS May I add you to the contributors?

[ijlyttle]

Could you rerun document()? TravisCI found a mismatch between this and the .Rd file. Thanks!

[ijlyttle]

PPS If you are re-running document(), could you add yourself to Contributors in DESCRIPTION? Thanks!

[ijlyttle]

Hi Alec,

I had a closer look at the function box_ls_short() that you are proposing, and while I like the idea of the functionality it would provide, I have a hesitation to repeat all of the code from box_ls().

It would solve my concern if, instead of adding a new function, we were to add an argument to box_ls(), fields = NULL.

If fields is NULL, all the fields would be returned. If fields is some subset of the available fields, only those fields would be returned. You may need some logic like:

filelds_all <- 
  c("modified_at" ,"content_modified_at", "name", "id", "type",
    "sha1" ,"size", "owned_by", "path_collection", "description")

if (is.null(fields)) {
  fields <- fields_all
} else {
  assertthat::assert_that(
    all(fields %in% fields_all),
    msg = paste("all fields must be in", paste(fields_all, collapse = ", "))
  )
}

In summary my proposal is to:

1. Add the fields argument to box_ls() (as well as @author)
2. Remove box_ls_short()
3. document()

What do you think? Let me know if you would like to take this on. If not, I can make the change after accepting the PR.

[ijlyttle]

For your email address to format properly, you will have to use @@.

[awong234]

Hi Ian,

Thanks for the tips! I would be happy to be listed as among the contributors.

I've integrated your suggestions in a new commit, and made the other changes you requested. Let me know if there is something missing.

[ijlyttle]

This was a typo on my part - should be fields_all.

Can you build-and-install on your end, just to make sure the function works as you expect?

In the longer term, I will have to look more-closely at the tests.

Thanks!

[ijlyttle]

I think you also need an entry at line 6 to document the fields argument, something like:

#` @param fields Default (`NULL`) returns all possible columns: 
#`  `"modified_at"`, `"content_modified_at"`, `"name"`, `"id"`, `"type"`, `"sha1"`,
#`  `"size"`, `"owned_by"`, `"path_collection"`, `"description"`. 
#`  To return a subset of columns, specify as a character vector.
#` 

If you document() again (and let me know that it works as you expect), I think we're there!

Thanks!

[awong234]

Oh right! Will do

[ijlyttle]

Hi Alec,

Just to be sure before I merge, box_ls() behaves as you expect using fields = NULL and fields = "name"?

[awong234]

Hi Ian,

Just tested functionality now, they return values as expected. Thanks!

[ijlyttle]

Thanks! 🎉