Fix to align()'s default align argument

[keithnewman]

I encountered an issue with the align() method when not setting an align argument and relying on the default value for the align argument. Usage from the align() docs:

align(
  x,
  i = NULL,
  j = NULL,
  align = c("left", "center", "right", "justify"),
  part = "body"
)

However, a few lines later, the match.arg applied to align has several.ok = TRUE, which means all four values are accepted as the align argument.

This becomes a problem when the number of columns is not a multiple of 4 as the align argument tries to recycle itself across columns. Newer versions of R will error when recycling over a non-multiple value. The example in the documentation didn't flag this because it is an example dataset with 4 columns.

A small example to highlight the issue is:

library(flextable)

# 4 columns is fine
ft4 <- flextable(head(mtcars)[, 2:5])
ft4 <- align(ft4, part = "all")

# 8 columns doesn't error, but is messy because
# length(align_value) != length(j)
# so we don't `rep` the `align_value` by length(i)
ft8 <- flextable(head(mtcars)[, 2:9])
ft8 <- align(ft8, part = "all")

# 5 columns is not okay
ft5 <- flextable(head(mtcars[, 2:6]))
ft5 <- align(ft5, part = "all")
#> Error in x[[property]]$data[i, j] <- value: number of items to replace is not a multiple of replacement length

^{Created on 2024-04-11 with reprex v2.1.0}

These changes to align() attempt to patch this and reinforce the testing of the align() method. For the testing, I've employed test snapshots to confirm the relevant styling does filter down correctly into the table. However this requires a move to testthat version 3. The overhead of transitioning code to testthat 3 makes this PR cluttered, so you may find it easier to review one commit at a time. The three commits are

1. Changes needed to move to testthat version 3
2. Fix and tests associated with patching align()
3. Running the DESCRIPTION file through usethis::use_tidy_description() (not required, but an easy win)

More details about the changes made, reasons why, and consequences are in the NEWS.md file.

[davidgohel]

Thanks you Keith,

I'm going to use a solution that keeps the parameter vectorised. Indeed, align can receive several values and this is a feature I don't want to kill:

ft <- flextable(head(mtcars[, 2:6]))
align_vals <- c("center", "right", "right", "right", "right")
ft <- align(ft, align = align_vals, part = "all")

Thanks also for the tests update and the DESCRIPTION reformatting. Unfortunately, that's a lot in one commit 😅 I will integrate most of them in separate commits.

[keithnewman]

Thanks David,

I see you have applied some of the fixes in 85e6227.

I'm going to use a solution that keeps the parameter vectorised. Indeed, align can receive several values and this is a feature I don't want to kill:

I'd like to reassure you that the changes in my pull request continue to allow the align argument to be vectorised. In fact, this feature was prominently described in the documentation changes to the align parameter of the align() function, and demonstrated in the examples added to the documentation and the extensive test scenarios added to tests/testthat/test-styles.R.

---

There is one part of my pull request that you did not copy over to 85e6227 that I should highlight the importance of. This was the transition from using

if (length(align_value) == length(j)) {

to

if (length(j) %% length(align_value) == 0) {

This was to address the second example in my original reprex, where the length of align can recycle without error but cause a messy result. For example:

ft <- flextable(head(mtcars[, 2:9]))
align_vals <- c("left", "right")
ft <- align(ft, align = align_vals, part = "all")

Provides no error or warning but yields this result.

The behaviour is understandable under the R framework: this is just an example value recycling between different length vectors. But this leaves you with a design decision where you should pick one of:

1. Embrace R's behaviour of recycling mismatched vector lengths by swapping

   flextable/R/styles.R

   Line 638 in 85e6227

   if (length(align) == length(j)) {

   to

     if (length(j) %% length(align) == 0) {

   which would yield the following result:
   
   However, are there many scenarios where a user benefits from specifying column alignments in this way? If not, then maybe you want to...
2. Error if align parameter does not have length equal to 1 or length(j).

---

Unfortunately, that's a lot in one commit 😅

I'm sorry for the size of the ab81bcc commit. Most of this was collateral to move to testthat version 3, which allows for snapshots to ensure the correct alignment appeared in the cells, but it appears you have a much neater and more compact strategy that uses an information_data_paragraph() function that I wasn't aware of.

The collection of tests added to tests/testthat/test-styles.R seem long but was beneficial to verify the expected behaviour in a variety of scenarios a user might throw at it. If you decide to take option 1 (allow recycling of align), the "ft6" example covers this scenario while also checking the behaviour in combination with a non-null j parameter value, which are not currently covered by your test suite. You may be able to recreate this example using your method for inclusion in your tests.

[davidgohel]

sorry, I misread your solution. And thanks, it's now integrated.