Skip to content

fix: extend spell check to cover .qmd files#71

Merged
d-morrison merged 3 commits into
mainfrom
claude/issue-69-20260516-0129
Jun 5, 2026
Merged

fix: extend spell check to cover .qmd files#71
d-morrison merged 3 commits into
mainfrom
claude/issue-69-20260516-0129

Conversation

@d-morrison

Copy link
Copy Markdown
Member

Fixes #69

The spelling R package only checks .R, .Rmd, and man pages by default, so .qmd files were never checked. This extends tests/spelling.R to also run spell_check_files() over all .qmd files using the existing WORDLIST.

Also fixes two typos now caught by the improved check: "oppenings" → "openings" and "anlyzing" → "analyzing".

Generated with Claude Code

The spelling R package only checks .R, .Rmd, and man pages by default,
so .qmd files were never checked. Extend tests/spelling.R to also run
spell_check_files() over all .qmd files using the existing WORDLIST.

Also fix the two typos now caught by the improved check:
- "oppenings" → "openings"
- "anlyzing" → "analyzing"

Co-authored-by: Douglas Ezra Morrison <d-morrison@users.noreply.github.com>
@github-actions

github-actions Bot commented May 16, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1-2-g6ad689f
Preview removed because the pull request was closed.
2026-06-05 00:53 UTC

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@d-morrison

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude finished @d-morrison's task in 1m 33s —— View job


Code Review

  • Gather context
  • Read changed files
  • Review code
  • Post findings

Overall this is a clean, well-scoped PR. The two typo fixes are correct and the new .qmd spell-check logic is solid. A few minor observations below.

opportunities.qmd

Both fixes are correct:

  • anlyzinganalyzing (line 19)
  • oppeningsopenings (line 66)

No issues.

tests/spelling.R

The new block is well-structured. Specific observations:

Good:

  • Handles missing inst/WORDLIST gracefully with the if (file.exists(...)) guard.
  • Handles the empty-file-list case with if (length(qmd_files) > 0).
  • Uses lang = "en-US", which matches the Language: en-US field in DESCRIPTION — consistent with how spell_check_test() picks up the language.
  • The comment explaining why spell_check_files() is needed (because spell_check_test() skips .qmd files) is a good use of a comment.

Minor: error-handling inconsistency

The existing check uses error = FALSE, which means spelling errors are printed but do not fail the test run:

spelling::spell_check_test(vignettes = TRUE, error = FALSE, skip_on_cran = TRUE)

The new .qmd check uses a hard stop(), which will fail the test run on any spelling error:

if (nrow(results) > 0) {
  print(results)
  stop("Spelling errors found in .qmd files")
}

If the intent is for both checks to behave the same way (fail CI on errors), the error = FALSE on the first call is likely a leftover from a project template and could be changed to error = TRUE for consistency. If error = FALSE is intentional (e.g., to allow currently-known errors in .Rmd/vignette files), the new .qmd behavior is intentionally stricter — which is fine since you've already fixed the errors caught by this check. Either way, a brief comment clarifying the intent would help future readers.

Fix this →

Nit: working-directory assumption

file.exists("inst/WORDLIST") and list.files(".", ...) are relative paths that assume tests are run from the project root. This is the standard convention for R packages and is already relied on by the existing code, so this is not a new concern — just worth being aware of.


Summary: The PR correctly solves the problem and the code is clean. The only actionable item is the error = FALSE / stop() inconsistency, which is worth addressing for clarity. Everything else is a nit or non-issue.

@d-morrison

Copy link
Copy Markdown
Member Author

Claude Code CLI (local session) is working on this — paws off until I'm done.

Address @claude review: clarify that the existing vignette/.Rmd check uses
error = FALSE (report-only, tolerate pre-existing issues) while the new .qmd
check uses stop() (hard fail) on purpose, since .qmd coverage starts clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@d-morrison

Copy link
Copy Markdown
Member Author

Addressed the review:

  • error = FALSE / stop() inconsistency: Added a comment documenting that the difference is intentional — the existing vignette/.Rmd check uses error = FALSE (report-only, so pre-existing/tolerated issues don't block CI), while the new .qmd check uses a hard stop() because .qmd spelling coverage is new and starts clean, so any new error should fail CI. Chose the comment route rather than flipping error = FALSE to TRUE, since changing the .Rmd/vignette behavior would expand this PR's scope and risk failing CI on untouched files.
  • Working-directory assumption (nit): Acknowledged as non-actionable — the reviewer noted it's the standard R-package convention already relied on by existing code, not a new concern.

@claude review

@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude finished @d-morrison's task in 26s —— View job


Code Review

  • Read changed files
  • Review addressed feedback
  • Post findings

The previous review's actionable item has been handled well.

tests/spelling.R — addressed feedback

The added comment (lines 2–5) clearly and accurately documents the intentional difference between the two behaviors:

# error = FALSE: the vignette/.Rmd check reports spelling problems but does
# not fail the test run, so pre-existing/tolerated issues there don't block
# CI. The .qmd check below intentionally uses a hard stop() instead: .qmd
# spelling is newly covered and starts clean, so any new error should fail.

This is exactly the right call — changing error = FALSE to error = TRUE would have expanded the PR's scope and risked failing CI on unrelated files. The comment gives future readers everything they need to understand the asymmetry. No further changes needed.


This PR looks good to merge.

@d-morrison d-morrison merged commit 115ce97 into main Jun 5, 2026
5 checks passed
@d-morrison d-morrison deleted the claude/issue-69-20260516-0129 branch June 5, 2026 00:52
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.

fix spell check workflow

1 participant