Add --no-update flag to spell-check

[TymekDev]

Closes #510

[lorenzwalthert]

LGTM, can you add a test?

[TymekDev]

Done! I went ahead and tweaked the test framework a bit to allow for checking post-hook state.

[TymekDev]

Hey, I have noticed that I have not generated roxygen beforehand. I couldn't get it fixed in time, though, because I ran into problems with additional_dependencies failing to install required packages. That I have sorted out by reinstalling R with rig instead of brew.

However, right now I am getting errors from pkgdown hook locally, because pkgdown is not installed.

1. I tried installing pkgdown globally, but that did not help.
2. I tried to specify additional_dependencies, but that didn't work because it is a script hook.
3. I tried to change it to r hook, but that didn't help, because it is still taken from the remote version of repository.

How do you suggest to approach this?

[lorenzwalthert]

I tried installing pkgdown globally, but that did not help.

Right approach.

I tried to specify additional_dependencies, but that didn't work because it is a script hook.

Yes

I tried to change it to r hook, but that didn't help, because it is still taken from the remote version of repository.

You should not change the hook type as a user (now you are a user if you want to commit in that repo and the hooks fail).

What is the error message you get exactly?

[TymekDev]

The error:

pkgdown..................................................................Failed
- hook id: pkgdown
- exit code: 1

Error in loadNamespace(x) : there is no package called ‘pkgdown’
Calls: loadNamespace -> withRestarts -> withOneRestart -> doWithOneRestart
Execution halted

I figured it out. I had an .Rprofile that activated renv within a checked out repository. This meant it skipped the global version and required installing pkgdown with renv. I have removed .Rprofile altogether and got that sorted out.

[lorenzwalthert]

I recommend emitting a message in these config files because as you did, one tends to forget

[TymekDev]

What do you think of specifying method = "radix" in order?

I couldn't help but notice that inst/WORDLIST after going through this hook is differently sorted compared to spelling::update_wordlist. The latter provides method = "radix" to sort. Due to that the changes to the inst/WORDLIST look substantial, because the sorting differs.

If this gets green light, would you like a separate PR for this change?

[lorenzwalthert]

I think you can default to invisible (the function) and avoid the if statement further down.

[lorenzwalthert]

actually no wait a sec.

[lorenzwalthert]

I don't think the function is necessarily an assertion. I think it makes more sense to introduce the notion of read-only hooks in the testing, as we already have it in the docs for every hook. Can run_test() gain a read_only argument and then you compare the input file and output file and assert it did not change if that flag is set? I know that's beyond this hook alone, so if you want, can you open a separate PR? Because as I commented in other PRs, I am inclined to be conservative with extention of the testing main function and it seems this addition is too general (function can do anything, but you really actually just want to check if contents are the same).

If you don't want to contribute the read_only change in the testing framework or not, I don't think we should add the post_hook_assert argument.

[TymekDev]

That sounds reasonable. To paraphrase: we add an additional assertion to hook_state_assert_one based on read_only argument that is derived from the nature of the hook? In this case, of testing spell-check, we would test it in both scenarios. Is that right? It might make sense to rename the flag to --read-only, so it is more generic and can be shared across other hooks.

I have started working on the change as I understood it and outlined above, however I keep running into roxygenize tests failing, even on a clean main branch.

── Failed tests ── Failed tests ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Error (test-hooks.R:394:1): (code run outside of `test_that()`)
<purrr_error_indexed/rlang_error/error/condition>
Error in `purrr::map2(path_candidate, path_candidate_temp, hook_state_assert_one,
    tempdir = tempdir, file_transformer = file_transformer, path_stdout = path_stdout,
    path_stderr = path_stderr, expect_success = expect_success,
    std_err = std_err, std_out = std_out, exit_status = exit_status)`: ℹ In index: 1.
ℹ With name: man/flie.Rd.
Caused by error:
! in/flie.Rd and /private/var/folders/lh/y7zmk_td7nzfjxxqxdwhy5r40000gn/T/RtmpLxX4J1/fileeaf15c5ae334/man/flie.Rd are not supposed to be identical but they are

[ FAIL 1 | WARN 0 | SKIP 1 | PASS 71 ]

Do you have an idea where that comes from?

[lorenzwalthert]

All great, exactly. This failure occurred as well in previous CI runs. Don’t know why. Try updating roxygen and other deps. Otherwise You can ignore it.

[lorenzwalthert]

I think this is still unresolved. Right?

[TymekDev]

Correct! I have a rough draft for the testing framework change, but haven't had an opportunity to clean it up and publish just yet.

[lorenzwalthert]

If this gets green light, would you like a separate PR for this change?

Uh, that I did not know. Yes please, separate PR.

[lorenzwalthert]

Now that I merged #529, @TymekDev you can resolve the merge conflicts here and we cna merge this as well. I realised that I prefer --read-only over --no-update, as I think it's more clear what it means for most users and would be consistent with our notion introduced in #529. Can you adapt it?

[TymekDev]

@lorenzwalthert done 👌

[lorenzwalthert]

Thanks, can you also update the vignette Available hooks to reflect the added argument?

[lorenzwalthert]

Great. Thanks. Wanna add some read only flags to some other hooks? Candidates are basically all write hooks. for styler, native dry run option could be used, roxygenize seems a bit more tricky…