-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Refactor encoding test helpers and use withr::local_locale() #5236
Conversation
"en_US.ISO8859-1", | ||
"en_US.ISO8859-15", | ||
"fr_CH.ISO8859-1", | ||
"fr_CH.ISO8859-15" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add more if we see skips on a system where there are non-UTF-8 locales available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I think I confused myself to not use withr::
because we don't use it in the code and only Suggests
it, but that's nicer.
closes #5231
I saw withr is already used elsewhere below |
@@ -53,7 +53,9 @@ DataMask <- R6Class("DataMask", | |||
|
|||
promises <- map(seq_len(ncol(data)), function(.x) expr(promise_fn(!!.x))) | |||
|
|||
env_bind_lazy(private$bindings, !!!set_names(promises, names_bindings)) | |||
suppressWarnings( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me feel anxious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it needs a comment about the kind of anticipated warnings. Also if this needs to be fixed here it might need to be fixed upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from @romainfrancois. I had asked a similar question in slack.
Alternative to #5231. withr does have support for locale change, so I used that + some probing for a non-UTF-8 locale.