DISCUSSION: Spell checks

[ddsjoberg]

Background Information

Can we discuss the spell checks? I see there is a workflow for this. But the spell checks are something I would expect to see in the test folder. If we can stick with the typical structure, I think we should. If not, it will be helpful to understand the current structure.

Definition of Done

No response

[bms63]

Moving to admiralci. @cicdguy can we move this to tests?

[cicdguy]

Up to you. Then you'll probably need to add spelling to the Suggests field, which isn't great.

[bms63]

ah - that is a good point...maybe CI check is the best as we just got rid of styler from suggests.

[ddsjoberg]

Good point @cicdguy about needing to add spelling to Suggests.

This is a pretty mild point of discussion: making the change or leaving it as it is are both fine. But could be a place where we can simplify the CI/CD workflow and move admiral to be more in-line with how the R community does it without the need to reference various renv profiles.

Perhaps related, @bms63 and I saw some strange Spelling workflow behavior in PR pharmaverse/admiral#1994 , where the Spelling workflow was failing. But when we added the misspelled words to the WORDLIST, the spelling package also removed "unused" words that resulted in the workflow failing.

[bms63]

I don’t think renv profiles are used in workflows?? @cicdguy am I mistaken?

I’m partial to leaving as is but Let’s just table for now and revisit after we simplify the branching strategy.

[cicdguy]

I think the renv profiles are used for containers and codespaces.

[galachad]

The test dir is not check for package usage in R CMD check, it should not display NOTES. We are able to add the spell check with requireNamespace("spelling", quietly = TRUE) so the test will be just skipped when the package is not existed and we do not need to add packages to Suggest.

Example:

test_that("spelling check", {

  skip_on_cran()
  skip_on_ci()
  skip_on_covr()

  if (!requireNamespace("spelling", quietly = TRUE)) {
    skip("The spell check was skip as count not found `{spelling}` package.")
  }

  pkg_dir <- rprojroot::find_package_root_file()

  spell_check <- spelling::spell_check_package(pkg_dir)

  paste(spell_check$word, collapse = ", ") |>
    testthat::expect_identical("", info = "These words needs to be fixed.")

})

I'm a fan of keeping all check in test, but I think in some cases, we should verify performance, e.g. checking all files with lintr can take a lot of time.

We can start with adding the spell check. Should these check be propagated from admiralci?

What do you think?

[ddsjoberg]

The standard workflow is to add a spelling.R file in admiral/tests folder. This is added using the usethis::use_spell_check() function. The file is similar to what @galachad outlined above.

if(requireNamespace('spelling', quietly = TRUE))
  spelling::spell_check_test(vignettes = TRUE, error = FALSE, skip_on_cran = TRUE)

By default the tests are skipped on CRAN. We can change error = TRUE to induce an error if the spell check fails during R CMD Check.

[galachad]

@ddsjoberg as spelling is separate ci job we should add skip_on_ci() to this test.

[ddsjoberg]

This could perhaps be an opportunity to simplify/reduce our CI/CD workflow? If we use the same structure suggested in the R Packages Book and implemented via usethis/spelling, the spelling CIs would no longer be needed and the spell checks would be wrapped in the R CMD Checks

[galachad]

I agree, in that case, we need to add {spelling} to Suggest or install dev dependencies individually in R CMD check job.