-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
GH-35949: [R] CSV File reader options class objects should print the selected values #35955
Conversation
I started on this as it'd be useful for some debugging I'm doing, though I wonder if there's a more efficient way of doing this than copying the code for each attribute. Perhaps not? |
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.
Seems like a good idea to me! This will almost certainly save you or somebody else some time in the future.
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.
Awesome! Just one more test and it's good to go!
@@ -574,3 +574,11 @@ test_that("open_delim_dataset params passed through to open_dataset", { | |||
|
|||
expect_equal(ds$time, "16-01-2023") | |||
}) | |||
|
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.
One more test here for the data access part (as opposed to the printing part)? Like:
test_that("CSV options field access", {
options <- CsvReadOptions$create(skip_rows = 102)
expect_identical(options$skip_rows, 102)
# ... for all the options we just added the ability to access
})
...for each option we added a
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.
Added
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.
Thanks! I made a ticket for the CI failure, which seems to affect every debian/testing-based job: #36030
Benchmark runs are scheduled for baseline = 7887605 and contender = fe60932. fe60932 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Fixes #35949