Release {precommit} 0.4.1

[lorenzwalthert]

Since {styler} needs to be updated on CRAN due to changes in parser error messages (r-lib/styler#1183), and #1176 breaks {precommit} tests, a new {precommit} release must preceed a new {styler} release.

Prepare for release:

• devtools::check_win_devel()
• Polish NEWS
• add tidy thanks.
• revdepcheck::revdep_check()
• update cran-comments.md
• run urlchecker::url_check(".")

Perform release:

• Create RC branch (for bigger releases)
• Bump version (in DESCRIPTION and NEWS)
• devtools::submit_cran()
• Approve email

Wait for CRAN...

• Tag release
• Merge RC back to master branch
• Bump dev version
• update CRAN speed change monitor PR.We have a new CRAN release coming up 🥳 See NEWS.md for new features.

[olivroy]

Suggested change

	std_err = ifelse(packageVersion("styler") < package_version("1.10.3"), "scope must be one", "`scope` must be one"),
	std_err = "scope", # wrong scope specified

Maybe it would be best to change this to only scope. Maybe rlang could change its error messages from rlang::arg_match() and this would break precommit again.

The tidyverse team advocates not to test error messages from other packages. I guess this applies to packages from the same maintainer?

[lorenzwalthert]

Maybe yeah. So should we just test that there is an error without checking the exact message? Like check for "" seems to work, NA and NULL don't do the trick. 😞
I think to be honest, since the next release is pressing, I'd probably do that now. In the mid to long term, I'd be interested in refactoring the testing infra. Would you be interested in supporting this? I created an issue and outlined how we could start that, shouldn't be too hard after all: #552

[olivroy]

So should we just test that there is an error without checking the exact message

that's the intention of the suggedtion here

I will see, but I was under the expression this was a regex, similar to when you use expect_error(). my suggestion was to simplify that regex to "scope" to avoid future change in the error message from styler affecting precommit. (mainly to avoid burden for future you)

[lorenzwalthert]

It is not a regex, we check if the string is a sub string of the error message, since fixed is TRUE. But regardless, we checking for scope would still work. I just went for the empty sting to eliminate any dependency on the error message. Because testing for scope still has a dependency, albeit a very minimal one.

[lorenzwalthert]

Test time needs to be reduced since on r devel for windows, it takes quite long.
I answered

Dear CRAN team

If I understand correctly, the concern is that the release candidate has an overall check time of 17 mins, which is exceeding 10mins on R devel for Windows.


I note that according to the check results at https://cran.r-project.org/web/checks/check_results_precommit.html (also see screenshot attached):

* the current release of my package takes 897s (15mins) on R devel for Windows, whereas it only takes 635s (10mins) on the release version of R. This is an increase of 50% of run time for the same package going from R release to R devel.
* the release candidate of my package takes 17 mins on R devel for Windows, which is insignificantly more than 15 mins compared to the previous release of my package on R devel.
* I conclude that checking the release candidate of my package on R devel is slow due to R devel being slower than R release, not due to a change in my package.


Yours 

Lorenz Walthert 

they said:

Thanks, we see:


Flavor: r-devel-windows-x86_64
Check: Overall checktime, Result: NOTE
 Overall checktime 17 min > 10 min


mainly from


* checking tests ... [11m] OK

Please reduce the test timings by using
- small toy data only
- few iterations
- or by running less important tests only conditionally if some environment variable is set that you only define on your machine?


Some fluctuation may be normal, and you should really stay below the 10 min threshold.

Best,

[lorenzwalthert]

The important tests to run on CRAN are those where

• a reverse dependency check of another package should fail (e.g. Failure with dev (now CRAN) roxygen2 #535)

We could also activate parallelisation, but the real gains on CRAN would probably be limited then.

Testing timing:

test-conda: skipped
test-config: 4s
test-docopt: 2s
test-exec: 3s (1 skipped)
test-hook-roxygenise: 5s
test-hook-regex-exclude: 2s
test-hooks: 46s
test-setup: 10s
test-utils: 3s

While the hook tests are obviously the core, they clearly take the longest and are not susceptible to breaking changes on CRAN, as we are using {renv} for isolation. That may change again in the future though. Probably makes sense to split up hook tests in one per hook file and then parallelise.