-
Notifications
You must be signed in to change notification settings - Fork 11
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
Encoding fixes #243
Encoding fixes #243
Conversation
Avoids putting created strings to Rcpp::String, otherwise we end up with something like this: `xml_node_create("a", "€"): "<a>\u0080</a>"`
The failing R 4.1 check complicates things a bit to much 😆 I'm not aware if it should work in whatever windows locale is selected. Maybe it works fine, because these characters are not available in the default "C" locale? |
hmmm, that does not work as expected :( |
Could you please have a look @jmbarbone ? Obviously I'm missing something here. It works as intended (that much I was able to confirm with a German locale R 4.2), I'm just unable to make the test work. |
Okay, the remaining error either seems to have nothing to do with us (something in testthats mclapply), or at least it only occurs in x32-R. I can live with that. Might poke it a few more times tomorrow, but if it cannot be fixed, I wont hesitate merging this either way and disable the oldrel Windows-CI. Somehow doubt that anybody was working on 32-bit R on Windows in the last decade. |
I will not investigate the build error until I have to.
After a restful sleep, I realized that I don't want to invest any more time in this problem and that it won't be an obstacle to the encoding PR I care about. We may need to investigate this once we roll out to CRAN. We may have to disable some testing on Windows. But right now there is a lot more to do and multiarch builds on Windows are something I am not that interested in. |
Noting that #244 is related to the test failures from this PR |
Thanks, still strange though. The x64 tests finished and locally I didn't recall issues either. There isn't anything I would suspect in this pull request either. |
Wasted another 2-3 hours on this, but to no avail. I dislike Windows for a reason and R on Windows makes no exception :) What I tried:
What I will try next:
|
I tried playing around with a few things but couldn't get any error to pop up. Maybe not as comprehensive as you've checked.
So backlog then? We can wait until it pops up again. |
Not the ideal way to proceed, but I have no other idea. At least it works with x64 and on R 4.2+ |
On non unicode systems, encoded strings must be passed through
Rcpp::String
to avoid broken encoding. On unicode systems there is no issue.Our previous attempt was not sufficient, this should be fixed now and the code now also a bit cleaner. Previously we checked our R locale and passed that along.
Obviously handling non unicode encoding is a major pita, but non unicode R and OSes will likely be around for a long time. We should also add some non unicode CI.