testthat icon indicating copy to clipboard operation
testthat copied to clipboard

Reinforce that skip is due to a package

Open MichaelChirico opened this issue 1 year ago • 10 comments

Have observed this with some packages that might not easily be detected as such, e.g.

skip_if_not_installed("parameters")
skip_if_not_installed("stats")

Alternatively, we could quote-wrap the name to make it clear the package is not a normal noun.

Incidentally fixing #1967

MichaelChirico avatar May 14 '24 21:05 MichaelChirico

Maybe use the {packagename} convention?

hadley avatar Oct 22 '24 22:10 hadley

Maybe use the {packagename} convention?

Sure, this is pretty identical to the reasoning now laid out in the style guide: https://style.tidyverse.org/documentation.html#package-names

If the package name might be misinterpreted as an ordinary word, disambiguate by following it with “package” or by wrapping the package name in {} (but not both).

Using { makes it less tempting to use paste() 😉

MichaelChirico avatar Oct 22 '24 23:10 MichaelChirico

(happy to update the other tests if we're aligned on the core fix)

MichaelChirico avatar Oct 22 '24 23:10 MichaelChirico

Yeah, I think so.

Although looking at again, I wonder if "cannot be loaded" should be "is not installed"? That's technically less correct, but I think almost every time a package fails to load is because it's not installed.

hadley avatar Oct 24 '24 16:10 hadley

I would keep the more technically correct wording, I have run into a few cases where requireNamespace() failed on an installed package and it can be very confusing to untangle if you're thinking "but it's installed!".

A more involved approach could be to surface the error message:

found <- tryCatch(find.package(pkg), error=identity)
if (inherits(found, "error")) skip("Not installed")
else if (!requireNamespace(pkg, quietly=TRUE)) {
  skip("Failed to load")
}

MichaelChirico avatar Oct 24 '24 17:10 MichaelChirico

Actually I think find.package() offers a nice way through here, added it in the latest commit.

MichaelChirico avatar Oct 24 '24 17:10 MichaelChirico

I like it!

hadley avatar Oct 24 '24 18:10 hadley

OK, great! I will tidy this up with passing tests later today or tomorrow.

MichaelChirico avatar Oct 24 '24 18:10 MichaelChirico

For #1967, I think the current change is already enough. Just the non-ASCII leading character { should be enough to wind up grouping these messages together, and it's probably enough to just have skip() messages citing packages all grouped together (i.e., even if a non-skip_if_not_installed() skip message uses {pkg} to start, it's good to have that grouped with the skip_if_not_installed() messages; the possibility a skip() message starts with { and is not related to a package name is ignored as very unlikely.

MichaelChirico avatar Oct 24 '24 19:10 MichaelChirico

Fine with me.

hadley avatar Oct 24 '24 21:10 hadley

Thanks for wrapping up! Slipped my mind 😵‍💫

MichaelChirico avatar Oct 29 '24 21:10 MichaelChirico

No problems!

hadley avatar Oct 29 '24 22:10 hadley