fread: Improve validation of the dec argument - #7738
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7738 +/- ##
=======================================
Coverage 99.01% 99.01%
=======================================
Files 88 88
Lines 17288 17291 +3
=======================================
+ Hits 17118 17121 +3
Misses 170 170 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| test(2370.5, yearmon(NA, format="character"), NA_character_) | ||
|
|
||
| ## validation of dec argument | ||
| test(7737.1, fread(input = "whatever.csv", dec = NA_character_), error="nchar(dec) == 1L is not TRUE") |
There was a problem hiding this comment.
this error comes from base, not data.table, so the test is fragile. Look around and especially near the top of the test file for our proposed way to structure such tests.
| } | ||
| stopifnot( is.character(dec), length(dec)==1L) | ||
| if (dec == "auto") dec = "" else stopifnot(nchar(dec) == 1L) | ||
| if (!is.na(dec) && dec == "auto") dec = "" else stopifnot(nchar(dec) == 1L) |
There was a problem hiding this comment.
this also gives the same "ugly" error for dec = letters, so might as well change to if (isTRUE(dec == "auto")) or if (identical(dec, "auto")).
There was a problem hiding this comment.
The case dec = letters is already handled by the stopifnot() just before it. But using one of your variations is nicer anyway.
ben-schwen
left a comment
ben-schwen
left a comment
There was a problem hiding this comment.
change LGTM, news is missing (before we can merge it)
|
It's ready to go from my side. |
| rm(inner, DT, DT2) | ||
|
|
||
| ## validation of dec argument | ||
| test(7737.1, fread(input = "whatever.csv", dec = NA_character_), error=base_messages$stopifnot("nchar(dec) == 1L")) |
There was a problem hiding this comment.
| test(7737.1, fread(input = "whatever.csv", dec = NA_character_), error=base_messages$stopifnot("nchar(dec) == 1L")) | |
| test(2382.1, fread(input = "whatever.csv", dec = NA_character_), error='dec= must be a single non-NA character, or "auto".') |
There was a problem hiding this comment.
Unfortunately, Github does not let me push this one. Test numbers need to be in ascending order because we check for this.
| rm(inner, DT, DT2) | ||
|
|
||
| ## validation of dec argument | ||
| test(2382.1, fread(input = "whatever.csv", dec = NA_character_), error='dec= must be a single non-NA character, or "auto".') |
There was a problem hiding this comment.
Apparently I mixed an extra , in. sorry for that.
Also can you move this as test(1439.1), adjust test 1439 and add the number of the issue of the comment of ## validation of dec argument
|
TY! |
3 participants
Fixes #7737 and adds a test.