Skip to content
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

Allow turning symbols off on UTF-8 platforms #29

Merged
merged 2 commits into from
Nov 2, 2017
Merged

Allow turning symbols off on UTF-8 platforms #29

merged 2 commits into from
Nov 2, 2017

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 2, 2017

Related: #27.

I'm not sure this is the best way to do this, but the original code didn't support turning it off at all (short of setting LC_CTYPE = "C").

@gaborcsardi
Copy link
Member

I might be missing sg, but this will not fix the problem, imo. I think the correct solution is

getOption("cli.unicode", l10n_info()$`UTF-8`)

no?

@codecov-io
Copy link

codecov-io commented Nov 2, 2017

Codecov Report

Merging #29 into master will increase coverage by 0.79%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #29     +/-   ##
========================================
+ Coverage      60%   60.8%   +0.8%     
========================================
  Files           9       9             
  Lines         375     375             
========================================
+ Hits          225     228      +3     
+ Misses        150     147      -3
Impacted Files Coverage Δ
R/utils.R 51.72% <100%> (ø) ⬆️
R/box-styles.R 100% <0%> (+4.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b07cd5f...965277b. Read the comment docs.

R/strrep.R Outdated
@@ -0,0 +1,8 @@
if (getRversion() < "3.3.0") {
strrep <- function(x, times) {
map_chr(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have map_chr

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay to add purrr-compat.R?

@gaborcsardi
Copy link
Member

Don't worry about the test failure. I am fixing those. It is a mess because if testthat now (almost!) parsing everything in UTF-8 and a parse() bug.

@gaborcsardi
Copy link
Member

Can you please just undo the test fix or remove the map_chr and use something like paste(rep(char, x), collapse = "") instead?

@krlmlr
Copy link
Member Author

krlmlr commented Nov 2, 2017

The test fix seems a consequence of my changes, can't undo without test failures. ?

@gaborcsardi
Copy link
Member

gaborcsardi commented Nov 2, 2017

That's fine, don't worry about test failures. I'll fix the tests, once testthat is fixed (r-lib/testthat#605)

@krlmlr
Copy link
Member Author

krlmlr commented Nov 2, 2017

Done.

@krlmlr
Copy link
Member Author

krlmlr commented Nov 2, 2017

(Although I'm fairly certain we still need to adapt the test like I did in 0ce2b5a.)

@gaborcsardi
Copy link
Member

Thanks! A rule test case is easy adapt like that, but more complicated output is hard. rebox() will take care of this, once testthat is correct. It actually does work fine with stable testthat, but dev testthat changed how it reads and parses files.

@gaborcsardi gaborcsardi merged commit ab8b54d into r-lib:master Nov 2, 2017
@krlmlr krlmlr deleted the b-option-pref branch March 25, 2018 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants