Need to recalculate ustr_maxlen after strings being converted to UTF8 #3451
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3451 +/- ##
==========================================
+ Coverage 95.13% 95.14% +<.01%
==========================================
Files 65 65
Lines 12303 12305 +2
==========================================
+ Hits 11705 11707 +2
Misses 598 598
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #3451 +/- ##
==========================================
+ Coverage 96.14% 96.14% +<.01%
==========================================
Files 65 65
Lines 12191 12193 +2
==========================================
+ Hits 11721 11723 +2
Misses 470 470
Continue to review full report at Codecov.
|
| on.exit({ | ||
| Sys.setlocale('LC_COLLATE', lc_collate) | ||
| Sys.setlocale('LC_CTYPE', lc_ctype) | ||
| }, add = TRUE) |
There was a problem hiding this comment.
Not sure if add=TRUE is appropriate here. Locale setting is properly reverted to old when local() finishes. add=TRUE make sense if there would be any other preceding on.exit calls inside local.
There was a problem hiding this comment.
It's the same with or without add=TRUE . They will be executed whenever the local() finishes. It's just a good practice (in my opinion) to always include this to avoid overriding the previous on.exist() logic, unintentionally.
Closes #3397.
ustr_maxlenshould be recalculated if there's any strings that need to be converted to UTF-8. Otherwise, it leads to wrong orders in certain cases. This PR fixes the example provided in #3397.TODO
find a test case that based onlatin-1- so that it can be checked on all platformsNo it's not possible (at least according to my experiments) to write a working case based purely on latin1 encoding.
Anyway, I added a test based on my original example, which should work on both Windows 7 and Windows 10 (tried on 3 machines - win7, win10, default locale English and Chinese - so it should be correct)