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

Suppress warning from {desc} in test #2794

Merged
merged 2 commits into from
Oct 18, 2024
Merged

Suppress warning from {desc} in test #2794

merged 2 commits into from
Oct 18, 2024

Conversation

maelle
Copy link
Collaborator

@maelle maelle commented Oct 7, 2024

No description provided.

Copy link

github-actions bot commented Oct 7, 2024

@hadley
Copy link
Member

hadley commented Oct 16, 2024

Maybe suppressWarnings() since we don't care whether or not there's a warning, we just want to silence it?

@maelle
Copy link
Collaborator Author

maelle commented Oct 17, 2024

Don't we want the test to fail when there's no warning in order to remove the comment that is just above it?

@hadley
Copy link
Member

hadley commented Oct 17, 2024

I don't think so because then if a new version of desc was released that removed the warning, then pkgdown would fail checks on CRAN. i.e. it creates an strong connection between the two packages for relatively little gain (i.e. at some point in the future someone will look at that code and be confused that there's no longer a warning).

@maelle
Copy link
Collaborator Author

maelle commented Oct 17, 2024

Thank you for the explanation, I updated the file.

@jayhesselberth jayhesselberth changed the title test: wrap known warning from {desc} into expect_warning() Suppress warning from {desc} in test Oct 18, 2024
@jayhesselberth jayhesselberth merged commit 8d9cba1 into main Oct 18, 2024
17 checks passed
@jayhesselberth jayhesselberth deleted the warn-desc branch October 18, 2024 01:11
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.

3 participants