Reduce use of pipe in codebase + refactor tests + linting

[olivroy]

Summary

Just some linting around the package

• Refactors some dplyr code into base R (improve performance)
• Linting using lintr::lint()
• regular expressions matching with fixed = TRUE
• Less magrittr pipe (performance / readability)

Checklist

• I understand and agree to the Code of Conduct.
• I have added testthat unit tests to tests/testthat for any new functionality.

Note: a render_unit() test should go on R CMD check, as I made a crucial mistake, but only test-coverage.yaml (and local testing) saved me

[rich-iannone]

This is great! On a related note, one thing I've been trying to do (slowly) is get rid of magrittr pipes in the codebase. There's not too many of them left and the advantage is that we could remove one dependency from the package. If you wanted to take that on here (or in another PR) it would be a great improvement.

[olivroy]

I have exactly this locally, I targeted especially the utils_xml file! Will update the PR in the next few days.

[olivroy]

I gotta say I don't really like (and am not super talented) fiddling with this type of code. (base R data subsetting and string manipulation)

but the performance improvements seem worth it

library(magrittr)
bench::mark(
  mtcars[mtcars$vs == 1,],
  mtcars |> dplyr::filter(vs == 1),
  mtcars %>% dplyr::filter(vs == 1)
)
#> # A tibble: 3 × 6
#>   expression                            min  median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>                        <bch:t> <bch:t>     <dbl> <bch:byt>    <dbl>
#> 1 mtcars[mtcars$vs == 1, ]             87µs  95.2µs     9687.   15.42KB    15.1 
#> 2 dplyr::filter(mtcars, vs == 1)      806µs 849.6µs     1072.    3.81MB    10.4 
#> 3 mtcars %>% dplyr::filter(vs == 1)   813µs 849.9µs     1076.    7.45KB     8.28

^{Created on 2024-05-16 with reprex v2.1.0}

For the review, I'd recommend you to take a look at commits individually.

The tests were incredibly useful as a safeguard. They caught my mistakes

I made mistakes in the commits that resulted in CI failures, so I'd say that these three commits were made in a hurry:
c6fa2a6, b153108, and 7c92c8d, but the rest of the commits should all be good to look at individually!

[rich-iannone]

Looks great to me, and thank you so much for your hard work on this!

[rich-iannone]

I looked through everything in this PR and agree with all of it. Also, didn't know that select() and distinct() could be replaced by one well-crafted distinct()! Will merge once CI fully passes.

[olivroy]

Thanks for looking it over!

Yes, but be careful distinct() (and arrange) is like mutate() (I have been bitten by it before), which means:

# doesn't work
mtcars |>
  distinct(x  = "vs")
# works
mtcars |> 
  select(x = "vs")
mtcars |>
  distinct(x = .data$vs)
mtcars |>
  arrange(.data$vs)
# deprecated
mtcars |>
  select(.data$vs)
# doesn't work
mtcars |>
  distinct(all_of("vs"))
# solution (with across's little friend)
mtcars |>
  distinct(pick(vs)) # works quoted too.
mtcars |> 
  distinct(pick(all_of("vs")))
# pick doesn't allow renaming with selection 
# doesn't work
  mtcars |> 
  distinct(pick(all_of(c(x = "vs"))))

The more you know!

[rich-iannone]

Wow, that’s potentially dangerous! But I trust you fully with any new-school distinct() work. :)