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.

Sign up for GitHub

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

Jump to bottom

Move requireNamespace() check for cyclocomp_linter() to the factory? #2777

Open
MichaelChirico opened this issue Feb 21, 2025 · 8 comments · May be fixed by #2785
Open

Move requireNamespace() check for cyclocomp_linter() to the factory? #2777

MichaelChirico opened this issue Feb 21, 2025 · 8 comments · May be fixed by #2785

Comments

@MichaelChirico
Copy link
Collaborator

Currently, the validity check for cyclocomp_linter() happens inside the lint loop:

lintr/R/cyclocomp_linter.R

Lines 25 to 33 in e673ee3

Linter(linter_level = "expression", function(source_expression) {
# nocov start
if (!requireNamespace("cyclocomp", quietly = TRUE)) {
cli::cli_abort(c(
"Cyclocomp complexity is computed using {.fn cyclocomp::cyclocomp}.",
i = "Please install the needed {.pkg cyclocomp} package."
))
}
# nocov end

I think it makes more sense to do so outside the loop, but simply moving it breaks many things in our test suite when {cyclocomp} is not available.

Here's one test in particular that gives me pause:

lintr/tests/testthat/test-linter_tags.R

Lines 55 to 74 in e673ee3

test_that("warnings occur only for deprecated linters", {
expect_silent(linters_with_tags(tags = NULL))
num_deprecated_linters <- nrow(available_linters(tags = "deprecated", exclude_tags = NULL))
deprecation_warns_seen <- 0L
outer_env <- environment()
expect_silent({
withCallingHandlers(
linters_with_tags(tags = "deprecated", exclude_tags = NULL),
warning = function(w) {
if (grepl("was deprecated", conditionMessage(w), fixed = TRUE)) {
outer_env$deprecation_warns_seen <- outer_env$deprecation_warns_seen + 1L
invokeRestart("muffleWarning")
} else {
w
}
}
)
})
expect_identical(deprecation_warns_seen, num_deprecated_linters)
})

Are we OK with just doing skip_if_not_installed('cyclocomp') here? Should we keep the requireNamespace() check where it is?
@AshesITR
Copy link
Collaborator

What about adding cyclocomp as a dev dependency? It's in our lint config anyway, no?

@MichaelChirico
Copy link
Collaborator Author

Yes, it's there. The issue is the R-CMD-check-hard CI which explicitly runs the suite absent Suggests deps.

@AshesITR
Copy link
Collaborator

Oh, I see. In that case I'm fine with skip_if_not_installed.
It there a way to guard us against the test being skipped everywhere?

@MichaelChirico
Copy link
Collaborator Author

One last wrinkle -- it's not so simple as just skip_if_not_installed("cyclocomp") -- there are also some chunks in the lintr.Rmd vignette that error out (from linters_with_tags() which tries to () the linters) and some examples that fail.

I still think it's the right thing to do but the diff will be bigger.

It there a way to guard us against the test being skipped everywhere?

As in, make sure that we don't accidentally let it start just getting skipped everywhere in CI, right? I think the default R-CMD-check use the CRAN defaults, i.e., require Suggests to be installed.

@AshesITR
Copy link
Collaborator

Yes, basically assert 100% test code execution for some environment.

One (probably very bad) idea:

Define

test_canary <- function(x) {
  x
}

and abuse code coverage statistics via

skip_if_not_installed("cyclocomp")
expect_equal(test_canary(42L), 42L)

@MichaelChirico
Copy link
Collaborator Author

MichaelChirico commented Feb 24, 2025 •
edited
Loading

One (probably very bad) idea:

Won't the existing codecov suite be enough? We'll get coverage there past any existing skip_if_not_installed('cyclocomp'), right? No need for dummy tests?

@MichaelChirico
Copy link
Collaborator Author

Here are the other 3 linters in our suite using requireNamespace:

lintr/R/literal_coercion_linter.R

Line 80 in 22ee8db

if (any(is_rlang_coercer) && !requireNamespace("rlang", quietly = TRUE)) {

lintr/R/object_overwrite_linter.R

Lines 58 to 62 in 22ee8db

for (package in packages) {
if (!requireNamespace(package, quietly = TRUE)) {
cli_abort("Package {.pkg {package}} is required, but not available.")
}
}

lintr/R/unused_import_linter.R

Line 98 in 22ee8db

if (pkg %in% except_packages || !requireNamespace(pkg, quietly = TRUE)) {

That makes cyclocomp_linter() unique in that the core functionality of the linter requires a specific package.

  • literal_coercion_linter() can optionally tailor its lint message if {rlang} is installed
  • object_overwrite_linter() can flag overwrites from user-specified packages, which it does in the factory (by default it only checks packages which should always be installed)
  • unused_import_linter() behaves worse when used on inputs importing certain packages that are not available

@AshesITR
Copy link
Collaborator

You're right, we would lose substantial code coverage if the codecov run would start skipping object_usage_linter. Therefor, it should be safe to guard all usages for R-CMD-check-hard.

@MichaelChirico MichaelChirico linked a pull request Feb 24, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move the cyclocomp namespace check into the factory r-lib/lintr
2 participants
@AshesITR @MichaelChirico