Support loading check.env #160

gaborcsardi · 2021-09-17T14:36:10Z

Will add docs...

Closes #12.

codecov-commenter · 2021-09-17T14:39:40Z

Codecov Report

Merging #160 (da337fd) into master (a809fd4) will increase coverage by 0.65%.
The diff coverage is 90.66%.

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
+ Coverage   80.85%   81.50%   +0.65%     
==========================================
  Files          18       19       +1     
  Lines         914      984      +70     
==========================================
+ Hits          739      802      +63     
- Misses        175      182       +7

Impacted Files	Coverage Δ
R/env.R	`87.93% <87.93%> (ø)`
R/background.R	`95.00% <100.00%> (+0.08%)`	⬆️
R/package.R	`95.12% <100.00%> (+0.06%)`	⬆️
R/parse.R	`93.60% <100.00%> (ø)`
R/utils.R	`87.77% <100.00%> (+1.52%)`	⬆️

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 a809fd4...da337fd. Read the comment docs.

jimhester · 2021-09-17T14:43:27Z

R/env.R

+  }
+}
+
+load_env_file <- function(path, envir = parent.frame()) {


Should this use readRenviron() instead of using our own implementation?

I guess the downside would be restoring the old envvars afterwards. But you could do Sys.getenv() to save them beforehand I guess.

I don't really like its way of handling errors, it just ignores them, and especially on a CI, it is better to know about them.

So it was easier to copy this code from another package of mine.

But I don't really mind, we can switch to readRenviron() as well.

hadley · 2021-09-17T15:51:09Z

This doesn't completely capture what I was looking for, which is some way to disable a standard set of notes that we don't think is important. i.e. this looks useful, but doesn't directly solve the problem I care about.

gaborcsardi · 2021-09-17T16:13:12Z

This doesn't completely capture what I was looking for, which is some way to disable a standard set of notes that we don't think is important. i.e. this looks useful, but doesn't directly solve the problem I care about.

I would wary about always ignoring these notes. E.g. the size note is definitely useful sometimes. With the check.env file, you can suppress the useless package size notes by setting a reasonable limit for a package.

But we can have that as well. Do you want to make it opt-in or opt-out? We can have a Config/rcmdcheck/ignore_notes flag for it?

It is better in tools/ because then it is not installed with the package.

gaborcsardi · 2021-09-18T08:49:51Z

@hadley now you can set

Config/rcmdcheck/ignore-inconsequential-notes: true

in DESCRIPTION. It sets these env vars currently:

  c("_R_CHECK_PKG_SIZES_" = "false",
    "_R_CHECK_RD_XREFS_" = "false",
    "_R_CHECK_CRAN_INCOMING_NOTE_GNU_MAKE_" = "false",
    "_R_CHECK_PACKAGE_DATASETS_SUPPRESS_NOTES_" = "false"
  )

Is this closer to what you had in mind?

- Always convert output to UTF-8. - Use the <xx> notation for bytes that are invalid in the native encoding. Closes #152.

hadley · 2021-09-20T12:20:44Z

Yeah, that's better. We can then change the check-full default to error with notes, and then there's a standard way to suppress either the most common unimportant notes, or selected notes affecting your package.

Then this PR just needs a bit of documentation so it's easy to find out these two ways of suppressing notes.

If the file is there, then load it, unless opt out.

gaborcsardi · 2021-09-20T15:21:32Z

@hadley @jimhester I have added some docs.

I also removed the dependence on NOT_CRAN. I think if somebody adds check.env and uses rcmdcheck, then they surely want to use it.

jimhester · 2021-09-20T15:41:49Z

I think we might still need to have some sort of non envvar based allow list. Even though CRAN has been adding more fine grained ennvars lately older versions of R won't have this, so we will still get false positive NOTES on those versions. This would preclude us from changing the default to failing on NOTEs for our action

name	versions
`_R_CHECK_PKG_SIZES_`	3.6.0+
`_R_CHECK_RD_XREFS_`	3.0.3+
`_R_CHECK_CRAN_INCOMING_NOTE_GNU_MAKE_`	3.6.0+
`_R_CHECK_PACKAGE_DATASETS_SUPPRESS_NOTES_`	3.3.0+

We are currently at 3.4 as the last version we are including in the checks, so it will still be awhile before we could use this unconditionally.

gaborcsardi · 2021-09-20T16:10:52Z

_R_CHECK_PKG_SIZES_ is included in R 3.4.x, I think, even in the R 3.0.x branch, actually. The other env var for the threshold is more recent.

As for the one for GNU make, yeah, that is a pity. But if it is the only one, then maybe it is not worth implementing a(nother) ignore mechanism for this single one.

We do use GNU make in a number of repos, so in these we could have error_on = warning on the older R versions. (I'll add support to set error_on from an env var.)

jimhester · 2021-09-20T16:54:21Z

Ah, yes you are right about _R_CHECK_PKG_SIZES_, it is _R_CHECK_PKG_SIZES_THRESHOLD_ that is only on 3.6+

gaborcsardi · 2021-09-20T17:13:39Z

@jimhester So do your think that it would be OK to add support for the error_on = "warning" workaround on older R versions? Only the packages using GNU make would need it. Then we don't need a generic ignore-list feature.

Unfortunately it is hard to do an ignore-list properly.

jimhester · 2021-09-20T18:28:54Z

yeah I think that is the best solution for now, we can maybe just fall back to using error_on: warning for packages that use GNU Makefiles.

Support loading check.env

7621c05

Closes #12.

gaborcsardi requested review from hadley and jimhester September 17, 2021 14:36

gaborcsardi mentioned this pull request Sep 17, 2021

Should we fail on R CMD check NOTES in check-full? r-lib/actions#381

Open

jimhester reviewed Sep 17, 2021

View reviewed changes

gaborcsardi added 2 commits September 17, 2021 21:39

inst/check.env -> tools/check.env

3a38158

It is better in tools/ because then it is not installed with the package.

Allow ignoring useless NOTES with Config/rcmdcheck/

2b68d5c

Handle encodings better:

2a1e1ab

- Always convert output to UTF-8. - Use the <xx> notation for bytes that are invalid in the native encoding. Closes #152.

gaborcsardi added 2 commits September 20, 2021 17:09

Do not consider NOT_CRAN for check.env loading

f4a0abe

If the file is there, then load it, unless opt out.

Document turning off checks

38a748b

gaborcsardi added 2 commits September 21, 2021 11:55

Document RCMDCHECK_LOAD_CHECK_ENV env var

898259e

Set error_on from env var by default

da337fd

gaborcsardi merged commit f0a7e3c into master Sep 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support loading check.env #160

Support loading check.env #160

gaborcsardi commented Sep 17, 2021

codecov-commenter commented Sep 17, 2021 •

edited

Loading

jimhester Sep 17, 2021 •

edited

Loading

gaborcsardi Sep 17, 2021 •

edited

Loading

hadley commented Sep 17, 2021

gaborcsardi commented Sep 17, 2021 •

edited

Loading

gaborcsardi commented Sep 18, 2021

hadley commented Sep 20, 2021

gaborcsardi commented Sep 20, 2021

jimhester commented Sep 20, 2021 •

edited

Loading

gaborcsardi commented Sep 20, 2021

jimhester commented Sep 20, 2021

gaborcsardi commented Sep 20, 2021 •

edited

Loading

jimhester commented Sep 20, 2021 •

edited

Loading

Support loading check.env #160

Support loading check.env #160

Conversation

gaborcsardi commented Sep 17, 2021

codecov-commenter commented Sep 17, 2021 • edited Loading

Codecov Report

jimhester Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

gaborcsardi Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

hadley commented Sep 17, 2021

gaborcsardi commented Sep 17, 2021 • edited Loading

gaborcsardi commented Sep 18, 2021

hadley commented Sep 20, 2021

gaborcsardi commented Sep 20, 2021

jimhester commented Sep 20, 2021 • edited Loading

gaborcsardi commented Sep 20, 2021

jimhester commented Sep 20, 2021

gaborcsardi commented Sep 20, 2021 • edited Loading

jimhester commented Sep 20, 2021 • edited Loading

codecov-commenter commented Sep 17, 2021 •

edited

Loading

jimhester Sep 17, 2021 •

edited

Loading

gaborcsardi Sep 17, 2021 •

edited

Loading

gaborcsardi commented Sep 17, 2021 •

edited

Loading

jimhester commented Sep 20, 2021 •

edited

Loading

gaborcsardi commented Sep 20, 2021 •

edited

Loading

jimhester commented Sep 20, 2021 •

edited

Loading