Added packets so no listing

muschellij2 commented 4 years ago

Added the packets functions so you can see what packets are available. Also removed some C++ code that didn't seem to do anything for record_indices. Rest are new roxygen2 changes to whitespace.

paulhibbing commented 4 years ago

I'm a little torn on this. My goal in listing them was for the options to be immediately visible in the documentation for read_gt3x. Apart from that, it seems odd to me to use a function just to return a static vector. From that perspective, an internal .packets variable might make sense, but then it's even harder for users to figure out what the options are. (And I don't know if experts balk at internal variables. I believe there are a few used in AGread, and as far as I'm concerned they make for more readable code...)

I may not be grasping the full rationale for your changes, so feel free to clarify.

muschellij2 commented 4 years ago

Rationale: 1) packets are listed at many spots - harmonizes this. 2) Any time I'm like "hey, what are the args again", I need to look at help or spec for the function, easier to print something out. 3) could use as a data object instead of a function. I'm fine with 3 instead of the functionized version I gave

paulhibbing commented 4 years ago

Alright. In that case here's how I would prefer to go about it:

In the file read_gt3x_utils.R, add the following (no documentation required):

    .packets <- c(
      "METADATA", "PARAMETERS", "SENSOR_SCHEMA", "BATTERY", "EVENT",
      "TAG", "ACTIVITY", "HEART_RATE_BPM", "HEART_RATE_ANT", "HEART_RATE_BLE",
      "LUX", "CAPSENSE", "EPOCH", "EPOCH2", "EPOCH3", "EPOCH4", "ACTIVITY2",
      "SENSOR_DATA"
    )

Leave the signature for read_gt3x as is, so the packet options are still visible for those new to the function.
For validate_include the choices argument can go away, and the test can be stopifnot(all(include %in% .packets))

I think that's the best tradeoff, in terms of accommodating users and developers. The options are then visible to developers using AGread:::.packets. (Or just as an object that gets loaded with devtools::load_all()).

I can add that myself, or you can switch it around and make it part of this PR. I don't know which way is better so I'll leave it up to you.

muschellij2 commented 4 years ago

Fixes #22 as well as provide functionality for reading in zipped gt3x files

paulhibbing / AGread

Added packets so no listing #21