Skip to content

Support R < 3.2#84

Merged
markvanderloo merged 5 commits into
markvanderloo:masterfrom
msberends:patch-1
May 21, 2021
Merged

Support R < 3.2#84
markvanderloo merged 5 commits into
markvanderloo:masterfrom
msberends:patch-1

Conversation

@msberends
Copy link
Copy Markdown
Contributor

This line in your init.R uses trimws():

term <- tolower(trimws(Sys.getenv("TERM")))

Unfortunately, this functions was only added in R-3.2, which makes this package unable to load in all versions of R-3.0 and R-3.1. This is a pity, since it's otherwise dependency free!

With this fix, users can test their package against older R versions.

@msberends
Copy link
Copy Markdown
Contributor Author

msberends commented May 17, 2021

By the way, the code is literally copied from base R 4.0 but works in all R versions since R-3.0 (the which and whitespace arguments were added in more recent versions):

base::trimws
#> function (x, which = c("both", "left", "right"), whitespace = "[ \t\r\n]") 
#> {
#>     which <- match.arg(which)
#>     mysub <- function(re, x) sub(re, "", x, perl = TRUE)
#>     switch(which, left = mysub(paste0("^", whitespace, "+"), 
#>         x), right = mysub(paste0(whitespace, "+$"), x), both = mysub(paste0(whitespace, 
#>         "+$"), mysub(paste0("^", whitespace, "+"), x)))
#> }
#> <bytecode: 0x7fc5cbc74120>
#> <environment: namespace:base>

@msberends
Copy link
Copy Markdown
Contributor Author

Added dir.exists() as well, supporting R 3.0

@markvanderloo
Copy link
Copy Markdown
Owner

Thank you for this. It was mentioned earlier by @bastistician in #77 and then I chose the easy way out :-).

@markvanderloo markvanderloo merged commit 8c8f142 into markvanderloo:master May 21, 2021
@msberends msberends deleted the patch-1 branch May 21, 2021 20:04
@msberends
Copy link
Copy Markdown
Contributor Author

Awesome!

Yes, remember that if you rely on only R > 3.0, package devs that want to support that version can use your package for unit testing, which will never happen to testthat.

With our AMR package for example, we support clinical decision-making in low income countries, as we understood from users who work for Doctors Without Borders. With this pull request, you now add to this support as well!! And that’s really, really great.

@markvanderloo
Copy link
Copy Markdown
Owner

That is great to know Matthijs. And in the spirit of open source! I also defined nullfile() for R versions < 3.6.0 now. (this is where graphics go when they are created in a test). Let me know if you run in to any more trouble for lower R versions.

@bastistician
Copy link
Copy Markdown
Contributor

Sorry for the late reply. There is an issue with

the code is literally copied from base R 4.0

because that code is Copyright (C) 1995-2018 The R Core Team under GNU GPL >= 2.
I think you need to keep that copyright notice if you redistribute the code.

@msberends
Copy link
Copy Markdown
Contributor Author

I don’t think that’s needed. This package is GPL-3 which is thus allowed, and the copied code was unchanged and already has the notice that is was copied from base R 4.0. But that my interpretation :)

@bastistician
Copy link
Copy Markdown
Contributor

As far as I can see, the code distributed in tinytest's R/utils.R source file is currently missing an appropriate copyright notice for the parts copied from (or based on) base R source code, e.g., trimws() taken from R's src/library/base/R/strwrap.R. GPL-2 (and 3) says that copyright notices must be preserved. Also, not acknowledging the original author amounts to plagiarism. I'd at least keep the one copyright comment line from the R sources, maybe saying something like "the following code is copied from / based on base::trimws in R 4.0.0 with Copyright (C) ..."

@markvanderloo
Copy link
Copy Markdown
Owner

Thanks, will add references before the next release.

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