Add support for show_col_types for edition 1 parser #1332
Conversation
|
I didn't add a test for this explicitly, but I can if you think it would be useful. |
There was a problem hiding this comment.
LGTM except my 1 suggestion re: a function-o.
I think it would be good to add some tests. Snapshot tests? I see readr is using testthat 3e, in terms of what DESCRIPTION says, but I haven't looked around at the actual usage.
Let me know if you want me to extend this PR with some tests.
|
Thanks for the fixes, and sure we can add some snapshot tests, you are welcome to refactor is_testing as needed to so you can do it. |
|
I will add some tests here. |
jennybc
force-pushed
the
edition-1-show-col-types
branch
3 times, most recently
from
2820735 to
0bb9c72
Compare
| #' * `FALSE` if the option is unset and we appear to be running tests | ||
| #' * `NULL` otherwise, in which case the caller determines whether to show | ||
| #' column types based on context, e.g. whether `show_col_types` or actual | ||
| #' `col_types` were explicitly specified |
There was a problem hiding this comment.
I reworded this to focus more closely on what this helper does. I still include some words about downstream use, in the NULL case.
| if (isTRUE(opt)) { | ||
| TRUE |
There was a problem hiding this comment.
This is new. But this just feels more natural and expected to me that if the option is TRUE or FALSE, that value is returned.
| TRUE | ||
| } else if (identical(opt, FALSE)) { | ||
| FALSE | ||
| } else if (is.na(opt) && is_testing()) { |
There was a problem hiding this comment.
I moved the is_testing() matter here.
| @@ -1,4 +1,3 @@ | |||
| pre_test_options <- options( | |||
| readr.show_col_types = FALSE, | |||
There was a problem hiding this comment.
I prefer to leave this unset and let should_show_types() fill in FALSE in the testing context, unless the option has been explicitly set.
`read_table()` is one of the functions that currently doesn't have a vroom equivalent, so is still using the first edition parser. When we added support for `show_col_types` we didn't port that back to the first edition parser, so it was missing in `read_table()` and the `read_delim_chunked()` functions. Fixes #1331
This is the helper we use in vroom, but I think having it in readr would be too confusing because we already have should_show_types
jennybc
force-pushed
the
edition-1-show-col-types
branch
from
b306fb6 to
63a2da4
Compare
| if ( | ||
| ((is.null(show_col_types) && !has_col_types) || isTRUE(show_col_types)) && | ||
| !inherits(ds, "source_string") | ||
| ) { |
There was a problem hiding this comment.
I moved the is_testing() matter into should_show_types().
read_table()is one of the functions that currently doesn't have avroom equivalent, so is still using the first edition parser. When we
added support for
show_col_typeswe didn't port that back to the firstedition parser, so it was missing in
read_table()and theread_delim_chunked()functions.Fixes #1331