Skip to content

#64 - implement NO_COLOR#65

Merged
gaborcsardi merged 5 commits intor-lib:masterfrom
nfultz:nfultz/64_NO_COLOR
Feb 8, 2018
Merged

#64 - implement NO_COLOR#65
gaborcsardi merged 5 commits intor-lib:masterfrom
nfultz:nfultz/64_NO_COLOR

Conversation

@nfultz
Copy link
Contributor

@nfultz nfultz commented Feb 8, 2018 •
edited
Loading

Quick and dirty implementation of NO_COLOR for #64

@gaborcsardi
Copy link
Member

gaborcsardi commented Feb 8, 2018

Thanks, looks good! Can you please add a test case, and also a bullet point to the NEWS?

@nfultz
Copy link
Contributor Author

nfultz commented Feb 8, 2018 •
edited
Loading

I don't understand why my test fails in R CMD check but not when I run the line manually in a terminal :(

It seems pretty obvious?

test_that("has_color, NO_COLOR=1", {
  withr::with_envvar(
    c(NO_COLOR="1"),
    expect_false(has_color())
  )
})

EDIT:

When I only run the has-color test, it passes, but when I run all tests it fails? very strange for such a simple test.

> devtools::test(filter="has-color")
Loading crayon
unloadNamespace("crayon") not successful, probably because another loaded package depends on it.Forcing unload. If you encounter problems, please restart R.
Loading required package: testthat
Testing crayon
Color detection: .................

DONE ===========================================================================
> devtools::test()
Loading crayon
unloadNamespace("crayon") not successful, probably because another loaded package depends on it.Forcing unload. If you encounter problems, please restart R.
Testing crayon
ansi-256: .....................
Colors and highlighting: .........
combine_styles: ......
Color detection: ................1
Does a string have ANSI style?: .
Strip style from string: ...
Making new ANSI 256 styles: ..........................................
String operations: ...................................................................................................................................................................................................................................................................................................................................................................................................................................................
General style function: ..
Defining new styles: S
utils: ..
Styling of character vectors: .........

Skipped ------------------------------------------------------------------------
1. new styles are local to importing package (@test-styles.r#6) - This is not implemented, yet.

Failed -------------------------------------------------------------------------
1. Failure: has_color, NO_COLOR=1 (@test-has-color.r#95) -----------------------
has_color() isn't false.


DONE ===========================================================================

After that, it fails for sure -

> devtools::test(filter="has-color")
Loading crayon
unloadNamespace("crayon") not successful, probably because another loaded package depends on it.Forcing unload. If you encounter problems, please restart R.
Testing crayon
Color detection: ................1

Failed -------------------------------------------------------------------------
1. Failure: has_color, NO_COLOR=1 (@test-has-color.r#95) -----------------------
has_color() isn't false.


DONE ===========================================================================

@nfultz
Copy link
Contributor Author

nfultz commented Feb 8, 2018

So I think one of the previous tests is setting the crayon.enabled option and not unsetting it on exit - I've worked around that for now but might be worth hunting down.

@nfultz nfultz changed the title WIP implement NO_COLOR #64 - implement NO_COLOR Feb 8, 2018
@gaborcsardi gaborcsardi merged commit 7008001 into r-lib:master Feb 8, 2018
@gaborcsardi
Copy link
Member

gaborcsardi commented Feb 8, 2018

Thanks!

@gaborcsardi gaborcsardi mentioned this pull request Feb 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nfultz @gaborcsardi