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

Work on R 3.1 #45

Closed
hadley opened this issue Apr 2, 2018 · 15 comments
Closed

Work on R 3.1 #45

hadley opened this issue Apr 2, 2018 · 15 comments

Comments

@hadley
Copy link

hadley commented Apr 2, 2018

It would be great to have R 3.1 support, since otherwise it's hard for us to use within tidyverse/R-lib.

@brodieG
Copy link
Owner

brodieG commented Apr 2, 2018

I understand, @gaborcsardi requested as much. Unfortunately, R_nchar was exposed between 3.1 and 3.2 in Rinternals.h, so I can't go back to 3.1 without a huge amount of work.

It might be possible to have a subset of the functionality that doesn't use width computations though, if that is helpful I'll consider it (although not right away as that will still be a bit of work with conditional compilation, etc.).

@gaborcsardi
Copy link

Could you call back to R's nchar on R 3.1? This is of course much slower, but if you only do it once or a few times, then maybe it is possible?

@gaborcsardi
Copy link

Btw. you need nchar(type = "width"), right?

@brodieG
Copy link
Owner

brodieG commented Apr 2, 2018

That's right, although I'm pretty sure that was available as of 3.1. I think using nchar will be difficult because right now the code is set up to process a stream of characters. In order to use nchar without completely crushing performance I'll need to do a two pass process that identifies all the non-ASCII characters and computes their width with one nchar call, and then shim that back into the existing stream logic. That is a bit of work and substantially increases the complexity of the code.

Of course, this is all doable, but I do wonder whether it is worth it. How long are you planning on supporting 3.1? 3.2 is about to have its third birthday, and 3.5 release is around the corner.

Two other bad options include literally calling nchar for every non-ascii character, which will be terrible performance wise, or writing custom grapheme width calculations (or more likely, integrating some of Pat Perry's work).

@brodieG
Copy link
Owner

brodieG commented Apr 2, 2018

A third option is to have a "crippled" version of the package in 3.1 which supports:

  • substr (but not width based)
  • strsplit
  • nchar/nzchar
  • sgr_to_html

as those don't require width calculations. The other functions could be made to work by assuming that every character is one wide. The package would warn via a startup message about its crippled state.

@hadley
Copy link
Author

hadley commented Apr 2, 2018

Unless you have some support for 3.1 (even if it's partial), we can't depend on it in cli, because cli is (or will be) used by the majority of packages in the tidyverse. At some point in the future, we will consider upgrading our base version to 3.2, but I think that should be a deliberate choice.

A crippled version for 3.1 is fine, as long as it doesn't crash and passes R CMD check. I'm not sure a package startup message is even needed.

@brodieG
Copy link
Owner

brodieG commented Apr 2, 2018

Ok I can do that.

@brodieG brodieG added this to the 0.2.3 milestone Apr 2, 2018
@gaborcsardi
Copy link

I think we can just turn off colors on R 3.1.x, in the tidyverse/r-lib packages.

@brodieG
Copy link
Owner

brodieG commented Apr 3, 2018

I think I'm mostly done with this (I'm just adding a stub R_nchar if R < 3.2). I do need to build 3.1 for testing, if that turns out to be more difficult than anticipated I'll let you know.

@gaborcsardi
Copy link

docker pull rocker/r-ver:3.1

???

@brodieG
Copy link
Owner

brodieG commented Apr 3, 2018

[sheepishly]Yeah, I guess I don't actually need to test this with valgrind instrumentation, do I?[sheepishly]

@gaborcsardi
Copy link

Yeah, nobody tests R 3.1 with valgrind....

@hadley
Copy link
Author

hadley commented Apr 3, 2018

Yeah, we can turn off colours, but we can't conditionally import a package (i.e. the main thing we care about is the depends field and passing R CMD check on R 3.1)

@brodieG
Copy link
Owner

brodieG commented Apr 4, 2018

This (development branch) now builds, installs, and runs on R 3.1, though do note the test suite is skipped, and width calculations for wide display characters (or even zero width characters) do not work properly.

https://travis-ci.org/brodieG/fansi/jobs/361921785

Please confirm this meets your requirements.

Since "Days since last update: 3" I will need to wait a couple of weeks before I can resubmit to CRAN.

@gaborcsardi
Copy link

Thanks, great!

@brodieG brodieG closed this as completed in 3f6c99c May 6, 2018
brodieG added a commit that referenced this issue May 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants