Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Reinforce that skip is due to a package #1959

Merged
merged 8 commits into from
Oct 29, 2024
Merged

Conversation

MichaelChirico
Copy link
Contributor

@MichaelChirico MichaelChirico commented May 14, 2024

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

@hadley
Copy link
Member

hadley commented Oct 22, 2024

Maybe use the {packagename} convention?

@hadley hadley added this to the v3.2.2 milestone Oct 22, 2024
@MichaelChirico
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@hadley
Copy link
Member

hadley commented Oct 24, 2024

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.

@MichaelChirico
Copy link
Contributor Author

MichaelChirico commented Oct 24, 2024

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
Copy link
Contributor Author

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

@hadley
Copy link
Member

hadley commented Oct 24, 2024

I like it!

@MichaelChirico
Copy link
Contributor Author

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

@MichaelChirico
Copy link
Contributor Author

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.

@hadley
Copy link
Member

hadley commented Oct 24, 2024

Fine with me.

@hadley hadley merged commit c843a3b into r-lib:main Oct 29, 2024
11 checks passed
@MichaelChirico MichaelChirico deleted the patch-7 branch October 29, 2024 21:50
@MichaelChirico
Copy link
Contributor Author

Thanks for wrapping up! Slipped my mind 😵‍💫

@hadley
Copy link
Member

hadley commented Oct 29, 2024

No problems!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants