GH-37994: [R] Create wrapper functions for the CSV*Options classes #37995
Conversation
|
|
github-actions
bot
added
Component: R
awaiting committer review
|
@github-actions crossbow submit preview-docs |
|
Revision: 1182e24 Submitted crossbow builds: ursacomputing/crossbow @ actions-fca1748e2e
|
|
We should rebase this after #38001 merges and incorporate changes from that PR too |
github-actions
bot
added
awaiting changes
r/R/csv.R
Outdated
| #' - `skip_rows_after_names` is applied (if non-zero). | ||
| #' | ||
| #' @export | ||
| csv_read_options <- CsvReadOptions$create |
There was a problem hiding this comment.
It may be worth considering doing this the other way around (include the body of the function here and assign CsvReadOptions$create <- csv_read_options), although I know there is precedent for doing it this way elsewhere in the package.
There was a problem hiding this comment.
(reasoning being that if you change a constraint or some validation of an argument, it's more obvious that the docs should be updated too)
There was a problem hiding this comment.
That's a great idea, swapped now!
r/R/csv.R
Outdated
| @@ -604,6 +647,22 @@ CsvParseOptions$create <- function(delimiter = ",", | |||
| ) | |||
| } | |||
|
|
|||
| #' CSV Parsing Options | |||
| #' | |||
| #' @param delimiter Field delimiting character (default `","`) | |||
There was a problem hiding this comment.
You might consider not putting the defaults here (because they are more likely to get out of sync with the code version of them below)?
There was a problem hiding this comment.
Good point, updated now!
github-actions
bot
added
awaiting changes
|
@paleolimbot I've made some additional changes since your last review - the one I'm not sure whether I should be moving to another PR or leaving in this one is the addition of |
github-actions
bot
added
awaiting changes
…th wrappers, reorganise what is mapped with what
thisisnic
force-pushed
the
GH-37994_csv_options
branch
from
3f6ff03 to
294ede4
Compare
github-actions
bot
added
awaiting review
github-actions
bot
added
awaiting merge
|
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 293819c. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…ses (apache#37995) ### Rationale for this change It's hard to find the docs for the R6 objects for the CSV reading/writing etc options classes ### What changes are included in this PR? Create wrapper functions, which are more easily documented ### Are these changes tested? Yep, I've swapped some existing tests to using the wrappers ### Are there any user-facing changes? Yes * Closes: apache#37994 Authored-by: Nic Crane <[email protected]> Signed-off-by: Nic Crane <[email protected]>
…ses (apache#37995) ### Rationale for this change It's hard to find the docs for the R6 objects for the CSV reading/writing etc options classes ### What changes are included in this PR? Create wrapper functions, which are more easily documented ### Are these changes tested? Yep, I've swapped some existing tests to using the wrappers ### Are there any user-facing changes? Yes * Closes: apache#37994 Authored-by: Nic Crane <[email protected]> Signed-off-by: Nic Crane <[email protected]>
…ses (apache#37995) ### Rationale for this change It's hard to find the docs for the R6 objects for the CSV reading/writing etc options classes ### What changes are included in this PR? Create wrapper functions, which are more easily documented ### Are these changes tested? Yep, I've swapped some existing tests to using the wrappers ### Are there any user-facing changes? Yes * Closes: apache#37994 Authored-by: Nic Crane <[email protected]> Signed-off-by: Nic Crane <[email protected]>
Rationale for this change
It's hard to find the docs for the R6 objects for the CSV reading/writing etc options classes
What changes are included in this PR?
Create wrapper functions, which are more easily documented
Are these changes tested?
Yep, I've swapped some existing tests to using the wrappers
Are there any user-facing changes?
Yes