add requires_utf8 argument to tests #7388
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7388 +/- ##
==========================================
- Coverage 98.97% 98.96% -0.02%
==========================================
Files 87 87
Lines 16733 16741 +8
==========================================
+ Hits 16561 16567 +6
- Misses 172 174 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Generated via commit e11d36d Download link for the artifact containing the test results: ↓ atime-results.zip
|
|
Sorry if I'm late to note this, but wouldn't a more reliable test for this be the same thing as we currently use for ñ in test 2266? A test may require some symbols (ñ, ü, ん) to be representable in the native encoding. The symbols may be represented using Unicode escapes ( |
Good point. I have integrated this for the utf8_check. |
ben-schwen
requested review from
MichaelChirico
and removed request for
MichaelChirico
| x1 = c("al\u00E4", "ala", "\u00E4allc", "coep") | ||
| x2 = c("ala", "al\u00E4") | ||
| tstc = function(y) unlist(lapply(y, function(x) as.character(as.name(x))), use.names=FALSE) | ||
| test(1088.1, requires_utf8="\u00E4", chmatch(x1, x2), match(x1, x2)) # should not fallback to "match" |
There was a problem hiding this comment.
maybe easier to understand/write tests as requires_utf8=c(x1, x2)?
also, maybe better as a local() check here too, to reduce the visual noise of all the identical requires_utf8= inputs?
| # for completness, include test from #2528 of non ascii LHS of := (it could feasibly fail in future due to something other than chmatch) | ||
|
|
||
| local(if (utf8_check("\u00E4")) { | ||
| eval(parse(text=' |
There was a problem hiding this comment.
why do we need eval(parse())?
also, parse(keep.source=FALSE) for micro-improvement
There was a problem hiding this comment.
When the parser sees constructs like
data.table("\u00FCber" = c(1, 0, 0, 0, 0))...it needs to construct a "language" (LANGSXP) call object where TAG(CADR(call)) is a symbol whose PRINTNAME is a CHARSXP saying über. That CHARSXP must be in the native encoding: requiring a single encoding makes it possible to compare pointers to SYMSXP values for equality (unlike CHARSXP where we have to test NEED2UTF8 and so on), and the native encoding was the default back before R had string encodings.
So when the parser tries to translate that Unicode string into the native encoding, it fails, emits a warning, and probably fails the following test, because the resulting string contains a substitution sequence:
LC_ALL=C R -q -s -e 'parse(text = r"{data.table("\u00FCber" = c(1, 0, 0, 0, 0))}")'expression(data.table(`<U+00FC>ber` = c(1, 0, 0, 0, 0)))
Warning message:
In parse(text = "data.table(\"\\u00FCber\" = c(1, 0, 0, 0, 0))") :
unable to translate '<U+00FC>ber' to native encoding
Without the runtime eval(parse(...)), this warning happens during source() with no way to avoid it.
There was a problem hiding this comment.
it would be good to write that down somewhere as a reminder, but I'm not sure the best place to do it while being (1) discoverable and (2) not repetitive.
maybe (1) document it near require_utf8 in R/test.data.table and (2) add a comment by each eval(parse()) like "see require_utf8 description"?
| eval(parse(text=' | ||
| DT = data.table(pas = c(1:5, NA, 6:10), good = c(1:10, NA)) | ||
| setnames(DT, "pas", "p\u00E4s") | ||
| test(1092, requires_utf8="\u00E4", eval(parse(text="DT[is.na(p\u00E4s), p\u00E4s := 99L]")), data.table("p\u00E4s" = c(1:5, 99L, 6:10), good = c(1:10,NA))) |
There was a problem hiding this comment.
nested eval(parse())... gnarly
3 participants
Closes #7336
Closes #1343
Closes #7333