Core Function Changes to Accommodate Empty Lists in Recursion, Covr Behavior on Windows & Vignette Path Warnings

[bburns632]

This PR makes changes to core functions and associated tests in three areas:

(1) Errors Caused by Control Statements Found During R6 and Normal Function Recursion

Full journey is documented in in #320, but in short:

• R6 made a change in their code, now present in 2.5.1.9000 R6#247.
• It contained the control statement break.
• This caused our recursive R6 method parser in FunctionReporter to arrive at an empty list when parsing our test package milne.
• Consequently, both R6 and regular function parsers have been updated to be tolerant of empty lists: .parse_R6_expression and .parse_function respectively.

(2) Covr Behavior on Windows

We run covr on a package that has already been loaded within the coverage functionality of CreatePackageReport(). This is fine on linux and mac, but breaks windows. Therefore, logic added to detach and reattach the package when running on windows. More on the issue from covr here: covr#443

(3) Checks for directory naming and location of a custom vignette_path

In CreatePackageVignette(), we provided the ability for a user to specify a custom vignette_path, the location of the file in which to output the vignette. We confirm the path and the extension are valid. However, we also check that the path ends in a vignette directory and warns if not.

This warning was correctly failing on windows devel tests (unsure why they were passing elsewhere). However, these checks are excessive. For a user that wants to specify a custom vignette file output, they likely have good reason to do so. Where that location may be does not need to be a concern. By confirm paths are valid and output paths in the terminal and log, we provide the user enough feedback to assess their own custom locations.

Therefore, these warnings and their associated checks were removed.

[jameslamb]

Thanks for the thorough description with links and the reasoning for these changes. I am totally on-board. I agree, the vignette path checks were excessive... {pkgnet} doesn't need to be in the business of also teaching people how to create R packages or keep up with the existing standards in the area of packaging vignettes.

I left some minor comments for your consideration, but overall I totally support this.

[jameslamb]

Yeah I agree this can be removed.

[jayqi]

The fix itself looks good.

However, I think we should add some new tests. Additionally, I think the PR description (and issue #320) could better document what the conditions are for the bug and what is being fixed.

From looking over things, it seems like the bug (that we've always had but just never run into) is that our function parsing didn't handle control statements like break. It seems next also behaves the same way.

is.symbol(quote(break))
#> [1] FALSE
as.list(quote(break))
#> [[1]]
#> `break`
is.symbol(quote(next))
#> [1] FALSE
as.list(quote(next))
#> [[1]]
#> `next`

^{Created on 2024-05-01 with reprex v2.0.2}

I don't see comments in #320 or anything in this PR's description that says something like "Fixes error in .parse_function and .parse_R6_expression when parsing function bodies that contain control statements like break and next." The "how we found it" (R6 added some code with break in it) is good to know but not the full story.

We have a bunch of tests for .parse_function for specific cases here and I think it would be good to add tests that contain for loops with break and next.

[bburns632]

@jayqi fair feedback. I added more to the notes and added a new test package, control, and associated tests to test for control statement handling. Adding a package was easier that adding a function to existing test packages and adjusting our expected stats.

@jameslamb I reverted some of your suggested refactor above as it was failing tests. Did not have time to address. Can tackle in a later PR.

[jameslamb]

I reverted some of your suggested refactor above as it was failing tests. Did not have time to address. Can tackle in a later PR.

ok sure, no problem!