Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Control color via an environment variable #126

Merged
merged 9 commits into from
Sep 9, 2021

Conversation

jimhester
Copy link
Member

No description provided.

R/styles.R Outdated Show resolved Hide resolved
R/styles.R Outdated Show resolved Hide resolved
@gaborcsardi
Copy link
Member

Thanks! I think we can simplify this, and get rid of the env var. rcmdcheck would just use the regular crayon color detection, plus it would turn it on for GHA (for itself only). Then it would turn color off for the checked package explicitly, in a similar way that testthat creates a reproducible context.

This will improve rcmdcheck in general, and we also don't need an env var.

@jimhester
Copy link
Member Author

Well, it is not just GitHub Actions that has this problem, GitLab has the same problem, which is why I used an environment variable so other places could control it.

I don't think rcmdcheck should really mess with the color in the actual R CMD check process, either turning it explicitly off or on.

@gaborcsardi
Copy link
Member

Well, it is not just GitHub Actions that has this problem, GitLab has the same problem, which is why I used an environment variable so other places could control it.

We can detect that as well. But we can also have an env var, I don't actually mind.

I don't think rcmdcheck should really mess with the color in the actual R CMD check process, either turning it explicitly off or on.

I think the reproducible context in testthat is really nice, and even base R is doing some of this when it is avoiding fancy quotes. I think it is the right thing to do to make R CMD check reproducible.

@jimhester
Copy link
Member Author

I just worry about introducing something into rcmdcheck that makes its behavior differ from R CMD check run on the command line

@gaborcsardi
Copy link
Member

I just worry about introducing something into rcmdcheck that makes its behavior differ from R CMD check run on the command line

Yeah, you are right, I realized this as well.

It would be nice to have some system that would standardize the context for the examples, that works for R CMD check itself. For the tests, the new testthat context is great.

@jimhester
Copy link
Member Author

I guess one option would be to have crayon detect R_TESTS https://github.com/wch/r-source/search?q=R_TESTS&unscoped_q=R_TESTS? That should always be set when R CMD check runs the tests.

For examples maybe it could check if the CheckExEnv (https://github.com/wch/r-source/blob/1349e5a0fd8a70ccef3a234527aca7495c4d9ff0/share/R/examples-header.R#L3) is on the search path?

@gaborcsardi
Copy link
Member

OK, this is in good shape. Let me fix the CI for the main branch first. Then I'll some docs for this, and we can merge it.

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2021

Codecov Report

Merging #126 (a068355) into master (cd0f365) will decrease coverage by 0.56%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
- Coverage   76.07%   75.51%   -0.57%     
==========================================
  Files          18       18              
  Lines         861      878      +17     
==========================================
+ Hits          655      663       +8     
- Misses        206      215       +9     
Impacted Files Coverage Δ
R/comparison.R 49.12% <ø> (ø)
R/error.R 0.00% <ø> (ø)
R/print.R 93.93% <ø> (ø)
R/styles.R 68.96% <62.50%> (-31.04%) ⬇️
R/comparison-summary.R 100.00% <100.00%> (ø)
R/utils.R 82.25% <100.00%> (+0.29%) ⬆️

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 cd0f365...a068355. Read the comment docs.

@gaborcsardi
Copy link
Member

Thanks much!

@gaborcsardi gaborcsardi merged commit 8f2a041 into r-lib:master Sep 9, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants